[Mono-dev] [PATCH] more support for Google Native Client
Elijah Taylor
elijahtaylor at google.com
Thu Jan 6 18:13:33 EST 2011
Hi Zoltan,
I've rebased my fork after your merge and subsequent fixes, and everything
seems to be working great with NaCl (I think the amd64 crash you were seeing
might have been the same one I was seeing). Thanks for the help with
reviewing the code and merging it in, I'm really glad we finally have these
changes upstream.
-Elijah
On Thu, Jan 6, 2011 at 11:50 AM, Elijah Taylor <elijahtaylor at google.com>wrote:
> Also, if any of the memory allocation routines don't get a larger alignment
> like pages, and instead just use MIN_ALIGN, yet pass in a larger alignment
> requirement to *mono_code_manager_reserve_align*, this code would be
> needed too. (sounds a bit contrived I guess :)
>
> On Thu, Jan 6, 2011 at 11:41 AM, Elijah Taylor <elijahtaylor at google.com>wrote:
>
>> Page alignment would be good enough, but I wasn't clear because I had
>> forgotten some details. When I saw this behavior we were using the "dummy"
>> implementation of mono_valloc which falls back on malloc, which I don't
>> believe we use anymore for the AOT compiler since it's a native app.
>>
>> This code is probably not necessary for NaCl anymore, but if anyone is
>> using this dummy implementation then it's still a valid fix.
>>
>>
>>
>> On Thu, Jan 6, 2011 at 11:31 AM, Zoltan Varga <vargaz at gmail.com> wrote:
>>
>>> Hi,
>>>
>>> Its fixed now, it was missing a (uintptr_t) cast around align_mask.
>>> mono_valloc () is supposed to return pagesize aligned memory, isn't that
>>> enough ?
>>>
>>> Zoltan
>>>
>>> On Thu, Jan 6, 2011 at 8:14 PM, Elijah Taylor <elijahtaylor at google.com>wrote:
>>>
>>>> This bit of code runs for our AOT compiler, but not for our JIT (in that
>>>> case, it's a native 32-bit app that defines __native_client_codegen__).
>>>>
>>>> The problem is that chunk->data isn't guaranteed to be aligned to
>>>> MIN_ALIGN **or** the alignment you pass into *
>>>> mono_code_manager_reserve_align* if you use mono_valloc instead of
>>>> dlmemalign in *new_codechunk*. But chunk->pos is aligned to the
>>>> alignment passed in, so the returned pointer could be misaligned.
>>>>
>>>> As you can see in the alloc function I adjusted it to give MIN_ALIGN - 1
>>>> extra bytes to account for this slop, and there's an identical piece to this
>>>> below for new codechunks that are allocated. I'm curious why this is
>>>> causing problems for non-nacl builds... if chunk->data is aligned this
>>>> should essentially be a no-op, and if it's not aligned, this code is
>>>> supposed to fix that. Is there a simple test I can run to see the failure?
>>>>
>>>>
>>>> On Thu, Jan 6, 2011 at 3:43 AM, Zoltan Varga <vargaz at gmail.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I had to revert this change, as it was causing crashes on amd64:
>>>>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>>>> @@ -357,8 +494,10 @@ mono_code_manager_reserve_align (MonoCodeManager
>>>>> *cman, int size, int alignment)
>>>>> for (chunk = cman->current; chunk; chunk = chunk->next) {
>>>>> if (ALIGN_INT (chunk->pos, alignment) + size <= chunk->size) {
>>>>> chunk->pos = ALIGN_INT (chunk->pos, alignment);
>>>>> - ptr = chunk->data + chunk->pos;
>>>>> - chunk->pos += size;
>>>>> + /* Align the chunk->data we add to chunk->pos */
>>>>> + /* or we can't guarantee proper alignment */
>>>>> + ptr = (void*)((((uintptr_t)chunk->data + align_mask) & ~align_mask)
>>>>> + chunk->pos);
>>>>> + chunk->pos = ((char*)ptr - chunk->data) + size;
>>>>> return ptr;
>>>>> }
>>>>> }
>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>
>>>>> it was inside a #ifndef native_client, so why is this needed ?
>>>>>
>>>>> Zoltan
>>>>>
>>>>> On Thu, Jan 6, 2011 at 11:34 AM, Zoltan Varga <vargaz at gmail.com>wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I merged your changes to mono's master except for the following:
>>>>>> runtime/mono-wrapper.in
>>>>>> mono/mini/genmdesc.c
>>>>>> nacl/
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>>
>>>>>> On Thu, Jan 6, 2011 at 2:22 AM, Elijah Taylor <
>>>>>> elijahtaylor at google.com> wrote:
>>>>>>
>>>>>>> Ok, I'll check out the changes/info you mentioned and go through the
>>>>>>> files that auto-merged, too. Probably won't get this done for at least a
>>>>>>> day or so, but I'll rebase again once I've fixed it. Hopefully by that
>>>>>>> point something else won't have broken too :)
>>>>>>>
>>>>>>> -Elijah
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jan 5, 2011 at 5:19 PM, Zoltan Varga <vargaz at gmail.com>wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This should work as follows: every aot image contains a
>>>>>>>> MonoAotFileInfo structure,
>>>>>>>> emitted in emit_file_info () in aot-compiler.c, which has a 'flags'
>>>>>>>> field, and the MONO_AOT_FILE_FLAG_FULL_AOT flag should be set in
>>>>>>>> this field. At runtime, check_usable() in aot-runtime.c checks this
>>>>>>>> flag.
>>>>>>>>
>>>>>>>> Zoltan
>>>>>>>>
>>>>>>>> On Thu, Jan 6, 2011 at 2:10 AM, Zoltan Varga <vargaz at gmail.com>wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Thu, Jan 6, 2011 at 1:24 AM, Elijah Taylor <
>>>>>>>>> elijahtaylor at google.com> wrote:
>>>>>>>>>
>>>>>>>>>> Zoltan,
>>>>>>>>>>
>>>>>>>>>> I've rebased from mono's master branch and fixed all merge
>>>>>>>>>> conflicts, but something that's gone in since I first forked has now broken
>>>>>>>>>> NaCl AOT compilation for me. On amd64 the compiler just crashes and I'm
>>>>>>>>>> looking into that, nut on x86 I'm getting this: Can't use AOT
>>>>>>>>>> image 'mscorlib' in aot-only mode because it is not compiled with
>>>>>>>>>> --aot=full. But I'm compiling with
>>>>>>>>>> --aot=full,static,nodebug,ntrampolines=4096
>>>>>>>>>>
>>>>>>>>>> If need be I can pick through the AOT changes that have gone in,
>>>>>>>>>> but I was hoping you or someone on this list would be able to tell me the
>>>>>>>>>> major changes to AOT from the past 3 weeks and some ideas about what might
>>>>>>>>>> be getting in my way. Can you shed any light?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> There was a big reorganization in the AOT file format to reduce the
>>>>>>>>> number of global symbols exported from the aot images. No idea why this is
>>>>>>>>> causing problems. make fullaotcheck and make fsacheck still seems to work
>>>>>>>>> for me on x86. I fixed a uninitilized memory error in 88d676ffd425def3,
>>>>>>>>> maybe that will help.
>>>>>>>>>
>>>>>>>>> Zoltan
>>>>>>>>>
>>>>>>>>>> -Elijah
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Jan 5, 2011 at 3:51 PM, Zoltan Varga <vargaz at gmail.com>wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I think the current code looks ok, and we should think about
>>>>>>>>>>> how to merge it into mono trunk. As a first step, could you rebase your
>>>>>>>>>>> master branch on top of master to fix the few conflicts which has surfaced
>>>>>>>>>>> due to changes to mono master ?
>>>>>>>>>>>
>>>>>>>>>>> Zoltan
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jan 5, 2011 at 8:23 PM, Elijah Taylor <
>>>>>>>>>>> elijahtaylor at google.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Zoltan,
>>>>>>>>>>>>
>>>>>>>>>>>> I've addressed all of the issues you pointed out (minus
>>>>>>>>>>>> genmdesc.c: __nacl_suspend_thread_if_needed, but that doesn't need to be
>>>>>>>>>>>> merged in at this time, it can remain in my local repository only). Please
>>>>>>>>>>>> take another look at your earliest convenience and let me know if there's
>>>>>>>>>>>> anything else you need from me.
>>>>>>>>>>>>
>>>>>>>>>>>> -Elijah
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Jan 4, 2011 at 10:55 AM, Elijah Taylor <
>>>>>>>>>>>> elijahtaylor at google.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Replies inline:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jan 4, 2011 at 10:30 AM, Zoltan Varga <
>>>>>>>>>>>>> vargaz at gmail.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Some comments:
>>>>>>>>>>>>>> - the patch changes IMT_REG to AMD64_R11 in the non-nacl case,
>>>>>>>>>>>>>> I'm not sure thats
>>>>>>>>>>>>>> intentional.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Has this changed in the last six months on the Mono side? IIRC
>>>>>>>>>>>>> I didn't mean to change anything like this. The reason I made explicit
>>>>>>>>>>>>> defines was so code in aot-compiler and mini-amd64 could share defines over
>>>>>>>>>>>>> which reg was the one we jump through and which was a scratch reg. I'll
>>>>>>>>>>>>> diff vs Mono head revision and make it correct.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> - you could define __mono_ilp32__ in the nacl/amd64 case, and
>>>>>>>>>>>>>> use that instead of
>>>>>>>>>>>>>> defined(__native_client_codegen__) && defined(TARGET_AMD64)
>>>>>>>>>>>>>> in a few places.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> That sounds reasonable. I'm assuming you mean non-arch
>>>>>>>>>>>>> specific areas like mini.c, aot-*.c, method-to-ir.c, etc? Are there any
>>>>>>>>>>>>> other major consequences to defining __mono_ilp32__ ?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> - it would be better to define nacl_global_codeman_validate ()
>>>>>>>>>>>>>> as a no-op in the non-nacl case, so its callers wouldn't need #ifdefs.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'll fix this.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> - genmdesc.c contains this change, which is probably not
>>>>>>>>>>>>>> needed:
>>>>>>>>>>>>>> +void __nacl_suspend_thread_if_needed() {}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> It is needed temporarily due to a preliminary GC
>>>>>>>>>>>>> implementation, we don't have to submit it this way. Eventually (soon) we
>>>>>>>>>>>>> won't need it at all.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> - you could use sizeof(mgreg_t) instead of SIZEOF_REGISTER to
>>>>>>>>>>>>>> be consistent with
>>>>>>>>>>>>>> the usage of sizeof(gpointer).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sounds good. I'll try to use sizeof for all compiled code and
>>>>>>>>>>>>> only use SIZEOF_REGISTER/SIZEOF_VOID_P for pre-processor directives only.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Other than these, I think the changes look fine, they aren't
>>>>>>>>>>>>>> that disruptive, since they don't
>>>>>>>>>>>>>> change the non-nacl behavior at all.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Great! I was worried just based on LOC changed that it might
>>>>>>>>>>>>> get more resistance. In truth I'm more worried about future Mono changes
>>>>>>>>>>>>> accidentally breaking NaCl behavior. I'm planning on getting some automated
>>>>>>>>>>>>> testing implemented soon to combat this though.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Dec 21, 2010 at 9:12 PM, Elijah Taylor <
>>>>>>>>>>>>>> elijahtaylor at google.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Greetings Mono developers!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *[tl;dr very large patch for Native Client<http://www.chromium.org/nativeclient> support
>>>>>>>>>>>>>>> hosted here <https://github.com/elijahtaylor/mono>, would
>>>>>>>>>>>>>>> love feedback and many eyes to look at it]
>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I'm back with another round of changes for supporting
>>>>>>>>>>>>>>> Google's Native Client (NaCl), including support for amd64, JIT compilation,
>>>>>>>>>>>>>>> and Garbage Collection. It's a large set of changes, forked on Dec 14 in
>>>>>>>>>>>>>>> github @ https://github.com/elijahtaylor/mono. I would
>>>>>>>>>>>>>>> appreciate feedback on these changes... to facilitate this, I'll try to
>>>>>>>>>>>>>>> explain the largest changes by feature (please email if clarification is
>>>>>>>>>>>>>>> needed):
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *1) amd64 codegen*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - Rules located here:
>>>>>>>>>>>>>>> http://www.chromium.org/nativeclient/design-documents/nacl-sfi-model-on-x86-64-systems
>>>>>>>>>>>>>>> - Removed %r15 from register allocation, LMF
>>>>>>>>>>>>>>> save/restore, etc. (r15 is special and not modifiable by untrusted code)
>>>>>>>>>>>>>>> - Sandbox all data access through membase address
>>>>>>>>>>>>>>> mode. If not %rsp or %rbp relative, re-write as clearing upper 32-bits +
>>>>>>>>>>>>>>> memindex addressing
>>>>>>>>>>>>>>> - align functions, call sites
>>>>>>>>>>>>>>> - Sandbox returns and all indirect jumps (need to be
>>>>>>>>>>>>>>> 32-byte aligned, cleared upper 32-bits)
>>>>>>>>>>>>>>> - Never omit frame pointer as general operations to
>>>>>>>>>>>>>>> rbp aren't allowed
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *2) NaCl x86-64 is ILP32 (this is the largest set of changes
>>>>>>>>>>>>>>> and may make some mono devs unhappy)*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - Set SIZEOF_REGISTER == 8 while sizeof(gpointer) == 4
>>>>>>>>>>>>>>> for NaCl amd64 (we can use 8-byte instructions, but pointers are 4-bytes)
>>>>>>>>>>>>>>> - Re-write large portions of mini-amd64.c, tramp-amd64.c,
>>>>>>>>>>>>>>> exceptions-amd64.c, mini.c, method-to-ir.c to use appropriate sizes
>>>>>>>>>>>>>>> (SIZEOF_REGISTER, sizeof(gpointer), literal '8'). *These
>>>>>>>>>>>>>>> changes are disruptive, but ultimately they should be more correct than what
>>>>>>>>>>>>>>> was there before. *It's our opinion that these changes
>>>>>>>>>>>>>>> actually improve Mono despite their impact.
>>>>>>>>>>>>>>> - We only generate NaCl amd64 code from an ILP32 machine
>>>>>>>>>>>>>>> (either a 32-bit application for AOT code, or NaCl runtime JIT), so we may
>>>>>>>>>>>>>>> not have caught all of the [8 <--> SIZEOF_REGISTER] conversions, but we
>>>>>>>>>>>>>>> likely caught most of the [sizeof(gpointer) <--> SIZEOF_REGISTER] and [8
>>>>>>>>>>>>>>> <--> sizeof(gpointer)] changes that are necessary.
>>>>>>>>>>>>>>> - Change atomic operations and default pointer directives
>>>>>>>>>>>>>>> to use 32-bit instructions (long instead of quad)
>>>>>>>>>>>>>>> - Change default operations to use 32-bit
>>>>>>>>>>>>>>> integers/pointers (eg, OP_LOAD_MEMBASE uses 4-bytes instead of 8)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *3) JIT support for NaCl*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - Since we're unable to emit code directly in its final
>>>>>>>>>>>>>>> executable location, we instead:
>>>>>>>>>>>>>>> - reserve a buffer on the heap
>>>>>>>>>>>>>>> - create a hash table entry mapping the temp location
>>>>>>>>>>>>>>> and final location
>>>>>>>>>>>>>>> - modify all non-local patches relative to the final
>>>>>>>>>>>>>>> location
>>>>>>>>>>>>>>> - request the NaCl runtime to install the created code
>>>>>>>>>>>>>>> in the final location
>>>>>>>>>>>>>>> - See mono/utils/mono-codeman.c changes for more detail.
>>>>>>>>>>>>>>> - For every codeman *reserve*, we must add a codeman *
>>>>>>>>>>>>>>> validate* call in order to install the
>>>>>>>>>>>>>>> method/trampoline/blob in the final location (as well as validate it for
>>>>>>>>>>>>>>> NaCl, pad it out, etc)
>>>>>>>>>>>>>>> - We don't delete or reuse code (we can, but it's icky
>>>>>>>>>>>>>>> and the benefits don't outweigh the cost)
>>>>>>>>>>>>>>> - Backpatching changed to use NaCl syscalls to modify
>>>>>>>>>>>>>>> existing dynamic code
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *4) GC support for NaCl (boehm only)*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - NaCl compiler and Mono code generator both emit
>>>>>>>>>>>>>>> instrumentation at GC "safe points" (back branches and function prologs),
>>>>>>>>>>>>>>> for cooperative thread parking (we're not allowed to send and receive
>>>>>>>>>>>>>>> signals)
>>>>>>>>>>>>>>> - Added new opcode OP_NACL_GC_SAFE_POINT to handle mono
>>>>>>>>>>>>>>> instrumentation
>>>>>>>>>>>>>>> - modified pthread_stop_world.c and pthread_support.c
>>>>>>>>>>>>>>> somewhat extensively to support this new way of stopping the world
>>>>>>>>>>>>>>> - wrapped pthread_exit because NaCl doesn't support
>>>>>>>>>>>>>>> pthread cleanup functions
>>>>>>>>>>>>>>> - added machine type NACL to libgc with machine specific
>>>>>>>>>>>>>>> defines
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *5) Misc bug fixes (not NaCl-specific)*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - fix *x86_memindex_emit* when disp is 32-bit
>>>>>>>>>>>>>>> - properly exclude code in libgc/gc_dlopen.c when
>>>>>>>>>>>>>>> DYNAMIC_LOADING not defined
>>>>>>>>>>>>>>> - properly exclude code based on DISABLE_SOCKETS by
>>>>>>>>>>>>>>> including config.h before checking define
>>>>>>>>>>>>>>> - clean up calculation of offset for amd64 AOT specific
>>>>>>>>>>>>>>> trampoline args
>>>>>>>>>>>>>>> - fix bug in *mono_bblock_insert_before_ins* when trying
>>>>>>>>>>>>>>> to insert an instruction to the beginning of an existing basic block.
>>>>>>>>>>>>>>> - fix small typo bug in genmdesc.pl which kept amd64 from
>>>>>>>>>>>>>>> being able to be a target of cross compiling
>>>>>>>>>>>>>>> - fix struct passing in amd64 with sizeof(struct) == 16
>>>>>>>>>>>>>>> when fields aren't 8-byte aligned (eg, first field is 12 bytes, second field
>>>>>>>>>>>>>>> is 4 bytes), pass on stack instead of in registers (mini-amd64.c:
>>>>>>>>>>>>>>> *add_valuetype*)
>>>>>>>>>>>>>>> - add extra checks to mini-amd64.c:*
>>>>>>>>>>>>>>> mono_arch_emit_exceptions* to keep exception/R4/R8
>>>>>>>>>>>>>>> emitting from overflowing a buffer silently
>>>>>>>>>>>>>>> - fix bugs in *new_codechunk* and *
>>>>>>>>>>>>>>> mono_code_manager_reserve_align* which allowed unaligned
>>>>>>>>>>>>>>> code to be allocated.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I know we're close to holidays so I don't have any delusions
>>>>>>>>>>>>>>> that these changes will get in by the end of the year :) Please feel free
>>>>>>>>>>>>>>> to pick apart these changes and let me know if there are things that should
>>>>>>>>>>>>>>> be changed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -Elijah
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>>> Mono-devel-list mailing list
>>>>>>>>>>>>>>> Mono-devel-list at lists.ximian.com
>>>>>>>>>>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20110106/60d6b4ee/attachment-0001.html
More information about the Mono-devel-list
mailing list