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 <elijahtay...@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 <elijahtay...@google.com>wrote: > >> Replies inline: >> >> On Tue, Jan 4, 2011 at 10:30 AM, Zoltan Varga <var...@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 >>> <elijahtay...@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@lists.ximian.com >>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list >>>> >>>> >>> >> >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list