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

[Maurizio Cimadamore]

Hi Henry,
nice work - some issues below:

* DescriptorParser - typo "outter" (both field and local have this issue)

* 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

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

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

* Value.java : Typo in "isNatviveByteOrder"

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

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) ?

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
>>
>>