[Mono-dev] [PATCH] more support for Google Native Client
Zoltan Varga
vargaz at gmail.com
Thu Jan 6 14:31:23 EST 2011
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/8e3e44b1/attachment-0001.html
More information about the Mono-devel-list
mailing list