[foreign] RFR 8218153: Read/Write of Value layout should honor declared endianness

[Henry Jen]

On Feb 25, 2019, at 3:06 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Hi Henry,
> nice work - some issues below:
> 
> * DescriptorParser - typo "outter" (both field and local have this issue)
> 

Will fix.

> * In "parseByteSwappedLayout" I think a try/finally would be in order, so that the code would be more resilient w.r.t. thrown exception and restoring the previous outer correctly
> 

You mean put the restoring to be in finally block? I will add that. I didn’t bother as the parse failed, I don’t think that layout should be trusted anyway.

> * It seems to me that the parser is still accepting the old lowercase/upper case syntax, this seems wrong
> 

OK, it's a misunderstanding. I was not sure if we want to keep this as I don’t see mentioning about remove. I kept it for compatibility and thinking it’s a nice shortcut for BIG_ENDIAN. I code as that uppercase take precedence. 

> * TestDescriptorGrammar looks borked - it should try both > and < - and not use upper/lower case letters
> 

Again, coded based on the assumption.

> * Value.java : Typo in “isNatviveByteOrder"
> 

Will fix.

> * no need for specializedLayout - just rename endian(Endianness) to withEndianness(Endianness), and that will do
> 

Sure. I was wondering if we need a method to strip out the endianness to NO ENDIAN. But decided not to asII don’t think we want to encourage doing that.

> More generally,  I would really like to see how this works with jextract changes too, and with stricter semantics applied to memory access in BoundedPointer::get/putBits. Without that kind of validation I'd be kind of reluctant to go ahead with this change, as there's basically nothing (other than the test you have added) stressing it.
> 

> Is there any reason in particular which pushed you to decouple the binder changes from the jextract changes (and, also, not to enforce the endianness check on memory write) ?
> 

The test case cover endianness access for short/int/long. Other perspective should have been covered, that’s why I think the test coverage should covering all path added.

The reason I didn’t do jextract is that it doesn’t have to, as I suggested, integral type in programming language has no endianness(as you considering “register”), so they are generated using default construction.

We can certainly take the step to enforce that endianness as jextract time, so that generated artifact can only be used on the same platform, which is the use case we are after anyway.

I didn’t enforce the memory access check because 1) I think that can be done later, as we need to change all test cases manually annotated and 2) I think that’s over-restricted	IMHO, a layout can be “specialized” at memory write time. It’s more a warning at read time if we think implicit is bad.

So I’ll update the webrev for above mentioned changes, and take another spin for adding the check.

Cheers,
Henry

> Maurizio
> 
> 
> 
> On 25/02/2019 21:00, Henry Jen wrote:
>> Updated webrev[1] to support byte swap operators, and enforce that the NativeMethodType can only have NE layouts.
>> 
>> Note that, we haven’t enforce the check at the descriptor parser level, which in theory we could, but that can be next step.
>> 
>> Also, we didn’t enforce explicit endianness check on “accessing memory”. However, a Value::specializedLayout can be used to make sure a layout have explicit endianness by make sure NE become host endianness, which I think is what we should do whenever a contents described by the layout is outbound to leave the process.
>> 
>> Cheers,
>> Henry
>> 
>> [1]http://cr.openjdk.java.net/~henryjen/panama/8218153/0/webrev/
>> 
>> 
>>> On Feb 3, 2019, at 12:16 PM, John Rose<john.r.rose at oracle.com>  wrote:
>>> 
>>> On Feb 3, 2019, at 12:04 PM, Maurizio Cimadamore<maurizio.cimadamore at oracle.com>  wrote:
>>>> …
>>>> I quite like this because it has a nice correspondence between layout API and description: in both cases there's no explicit support for NE - which is encoded always as a lack of explicit endianness info.
>>> Well, I like it too.  Thanks for teasing out the details.
>>> 
>>> One other advantage:  The NE state can naturally be upgraded
>>> after the fact to BE or LE (or vm-specific PE of course).  This means
>>> that we have more options for layout strings produced by jextract
>>> or by hand:  They can be conveniently left in the NE state, and
>>> the BE or LE state can be pushed into the layout string from context.
>>> 
>>> This gets an effect very similar to what Henry wants, which is a
>>> sensible default for simple notation.  It gets this effect not by
>>> eagerly assigning PE to elements in a layout string (which I
>>> don't want) but rather allowing a chance for the context of the
>>> layout string to supply then needed polarity.  The polarity
>>> can be explicit in a central place, such as in a global argument
>>> sent from jextract to the binder.  For casual use with PE, the
>>> polarity can be quietly injected as an optional argument to the
>>> function which parses layouts.
>>> 
>>> The basic idea behind this, of NE, is that some layouts can
>>> (and should) be "endian pure" in refusing to take a side, and the
>>> endian-ness can be specified where it is needed, in order to
>>> form correct memory accesses.
>>> 
>>> Thanks, guys.
>>> 
>>> — John
>>> 
>>>