[Mono-dev] Proposed Patch - Google Native Client

[Elijah Taylor]

Hi Zoltan, comments below inline.  I'll update with an new diff early next
week to address the feedback.

On Fri, Jul 2, 2010 at 12:41 PM, Zoltan Varga <vargaz at gmail.com> wrote:

> Hi,
>
>   I finally managed to read the codegen specific changes, and here are my
> comments:
> - I'd prefer the nacl related changes to be in as few places as possible,
> so later changes by
>   people who don't know nacl won't break nacl support.
>

Agreed.  I'd like the diffs to be small so NaCl support doesn't break and
also so the changes are as easy to understand as possible.


>   There are three places where this is not done:
>   - x86_prefix - this might be unavoidable
>

Yes, I wasn't sure what to do about this, but I don't like it either.
 Luckily it's in very few spots.  Unfortunately it means that anyone writing
new code that uses x86 prefixes will likely not write NaCl-compliant code,
and what's worse, it will be a really obscure bug if it does show up,
because it will only occur when the instruction that follows it along with
the prefix would straddle a 32-byte boundary.  This error would show up as a
NaCl validation error, so at least there shouldn't be any runtime bug
exposed with this.

One possibility is to pad out all x86_prefix instructions to the nearest
32-byte boundary, but that could really bloat things depending on how often
they're used.  Do you have any idea of the prefix to non-prefix instruction
ratio?  It seems like it'd be pretty low based on looking at the code but I
haven't looked at any actual metrics.



>   - the calls to nacl_pad_call_reg/call_membase (). Could this be folded
> into
>     x86_call_reg/x86_call_membase () ?
>

Agreed. This is what I've been experimenting with on x86-64 already (mostly
because of the variable length of the call sequences), I would like to do
the same on x86.  Essentially have pre/post calls in those macros that align
the call and pad it out correctly based on the actual length of the call
emitted, it also gets away from having to hard-code the length of various
call sequences which is fragile.


>   - places the where code is later patched. I think it would be better to
> modify
>     x86_patch () so it skips the padding added by nacl so these changes are
> not needed.
>

I did modify x86_patch to skip the nops emitted by x86_padding, so I didn't
need to align simple inline branch patterns, but I can't recall the specific
reason off the top of my head why I still needed to align some of these
instructions prior to adding a patch info.  It might have been related to
mono_arch_get_patch_offset, which I did not modify to skip nops.  I'll
experiment with this more to see if we can lose these extra instructions.


>
>                                                                   Zoltan
>
> On Tue, Jun 22, 2010 at 7:29 PM, Elijah Taylor <elijahtaylor at google.com>wrote:
>
>> Greetings Mono Developers,
>>
>> Attached is a patch to support 32-bit x86 code generation for Google
>> Native Client (http://code.google.com/p/nativeclient/).  I encourage you
>> to browse our project for more information if you're curious.  I apologize
>> for the large diff, let me try to explain the highlights to make it easier
>> to digest.
>>
>> There is a code generation component (define: __native_client_codegen__)
>> which affects the Mono bytecode -> native code generation for x86-32.  There
>> are a set of alignment restrictions, illegal instructions, and replacement
>> instructions we use for Native Client to ensure proper control-flow
>> sandboxing.  Please see
>> http://nativeclient.googlecode.com/svn/data/docs_tarball/nacl/googleclient/native_client/documentation/nacl_paper.pdffor more details.
>>
>> There is also a runtime component (define: __native_client__) which
>> modifies or disables some functionality to be compatible with the Native
>> Client runtime.
>>
>> We also had to modify some code that doesn't fall under either of the
>> above defines.  Most of these changes revolved around type safety.  The
>> modified version of gcc we use to compile Native Client modules is more
>> strict about types, and it caught what look like legitimate issues with the
>> Mono codebase.  The largest issue in terms of number of errors was the use
>> of mono_bool and gboolean interchangeably between declaration and definition
>> of many functions.  gboolean is defined as an "int" but mono_bool is defined
>> as int32_t.  Other type issues are listed directly below.  Feedback is
>> appreciated on these changes because of our unfamiliarity with this code,
>> but I modified these in the way that seemed most "right" at the time.
>>
>> mono/metadata/decimal.h:47 mono_decimal2string int -> gint32
>> mono/metadata/filewatcher.h:28 gboolean -> int
>> mono/metadata/filewatcher.c:158 int32 -> gint32
>> mono/metadata/threads-type.h:64 int -> gint32
>>
>> mono/mini/mini.h:1546  gboolean sort_end -> int sort_type
>> mono/mini/mini.h:1733  gboolean fp -> int bank
>>
>> The last bit of modification is to genmdesc and the Makefiles in general.
>> We added a new flag to genmdesc called "nacl" which overrides the given max
>> length of an instruction.  Native Client code tends to be larger because of
>> some of the instruction requirements we have, so some of the instructions in
>> cpu-x86.md had to be modified.  This is all tied to a new configure flag
>> called "--enable-nacl-codegen", which enables the codegen define, and sets
>> up calls to genmdesc with a --nacl flag.  It also modifies the mono-wrapper
>> script so one aspect of our code generation rules (masking jump targets to
>> 32-byte boundaries) is turned off while compiling and testing mono from the
>> Makefiles, which is required when testing outside of the Native Client
>> environment.  We're also including a standalone check "fsacheck" which tests
>> mono code generation as full AOT and a the library linked into a fully
>> static executable.
>>
>>
>> I look forward to your comments, questions, and suggestions.
>>
>>
>> -Elijah Taylor
>> Google Native Client Team
>>
>> _______________________________________________
>> 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/20100702/8ec517f8/attachment-0001.html