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 <[email protected]> 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 <[email protected]> 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 <[email protected]>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 <[email protected]> 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 <[email protected]> wrote: >>>> >>>>> Hi, >>>>> >>>>> On Thu, Jan 6, 2011 at 1:24 AM, Elijah Taylor <[email protected] >>>>> > 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 <[email protected]>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 < >>>>>>> [email protected]> 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 < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Replies inline: >>>>>>>>> >>>>>>>>> On Tue, Jan 4, 2011 at 10:30 AM, Zoltan Varga <[email protected]>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 < >>>>>>>>>> [email protected]> 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 >>>>>>>>>>> [email protected] >>>>>>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ Mono-devel-list mailing list [email protected] http://lists.ximian.com/mailman/listinfo/mono-devel-list
