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 <[email protected]> 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 <[email protected]>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 <[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
