[Mono-dev] [PATCH] more support for Google Native Client

Elijah Taylor elijahtaylor at google.com
Thu Jan 6 14:50:43 EST 2011


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/3827b834/attachment-0001.html 


More information about the Mono-devel-list mailing list