[Mono-dev] Proposed Patch - Google Native Client

[Elijah Taylor]

Hi, comments inline:

On Mon, Aug 2, 2010 at 5:00 AM, Zoltan Varga <vargaz at gmail.com> wrote:

> Hi,
>
>   Some further comments:
>
> - it would be nice to follow the mono coding conventions for the changes,
> i.e.
>   put a space before '(' in calls, use tabs in files where tabs are used
> for indentation, etc.
>

Yes, we've been trying to do that for future modifications in amd64.  I can
address these issues with x86 with an alternate patch if you'd like, let me
know.


> - some of the #ifdefs could be avoided by adding a NACL_SIZE() macro:
>
> #if defined(__native_client_codegen__) || defined(__native_client__)
> #define NACL_SIZE(a, b) (b)
> #else
> #define NACL_SIZE(a, b) (a)
>
> and using it for things like the trampoline buffer sizes:
>
> +#ifdef __native_client_codegen__
> + tramp_size = 128;
> +#else
>   tramp_size = 64;
> +#endif
>
> would become:
>
>        tramp_size = NACL_SIZE (64, 128);
>
>
It looks like you're trying to optimize for as few preprocessor defines as
possible.  If that's the case, we'll try to carry that sentiment over to our
new changes too.

One thing we've been talking about locally is what to do in amd64 (where
we're having to make more changes than in x86) when we have divergent
behavior between normal mono and NaCl.  Trying to be good guests in the Mono
codebase, we started putting the mono codeblocks first, like so:

#if defined(__default_codegen__)
//normal mono behavior
#elif defined(__native_client_codegen__)
//new nacl behavior
#else (if needed)
//optional default case
#endif

And then defining __default_codegen__ for non-nacl builds.  Obviously this
is a little messy as it adds a new define for every other build, but we
thought it made it clear that it is the default path, rather than using
"#ifndef __native_client_codegen__".  I find that it's easier to parse code
with a lot of ifdefs rather than a mixture of ifndefs and ifdefs.

Does anyone have any strong opinions one way or another about how we do
this?


                             Zoltan
>
>
> On Fri, Jul 16, 2010 at 1:30 AM, Elijah Taylor <elijahtaylor at google.com>wrote:
>
>> Hi, here's an updated patch with your feedback addressed.  I re-based the
>> diff closer to head revision (r160382) to include the other changes of ours
>> that already landed, as well as make sure we're still compatible with
>> current Mono development.
>>
>> In general this diff should have a smaller impact on the .c files:
>> mini-x86.c, exceptions-x86.c, tramp-x86.c specifically, and the Native
>> Client changes are a little more grouped together rather than spread out.
>>
>> A couple of points separate from the feedback:
>> 1) I fixed a bug in my implementation of genmdesc.pl changes, so that
>> will be different from the previous patch
>> 2) There's a small typo at head revision in mono/mini/tramp-x86.c which
>> says "rethow" instead of "reth*r*ow" for your rethrow exception
>> trampoline.  This is also fixed in my patch.
>>
>> As always feedback is appreciated from everyone.
>>
>>
>> -Elijah
>>
>>
>> On Tue, Jul 6, 2010 at 6:35 AM, Zoltan Varga <vargaz at gmail.com> wrote:
>>
>>> Hi,
>>>
>>>
>>>> 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.
>>>>
>>>>
>>> I think that would be ok, they are seldom used.
>>>
>>>                                  Zoltan
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20100802/6d6ab322/attachment-0001.html