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

[Maurizio Cimadamore]

Looks good - I have some (minor) points/questions:

* How comfortable are we to always infer platform endianness for Address 
layout? Should that be explicit too? (while for real pointers that 
probably doesn't matter, for protocol schema addresses, an address could 
be just some index, hence stored in LE/BE format?)

* We could have utility helper methods for extracting signedness and 
byte-reversal booleans out of a Layout - the same code is replicated in 
a number of places

* I'd like to understand more about DirectNativeInvoker limitations 
(mostly because this will carry over to linkToNative too) - what exactly 
goes wrong if we allow mismatching endianness there? Let me unpack this 
a bit: the direct invoker is only concerned in those calls that occur 
via registers only - why is endianness a factor here - since we are 
never storing into memory?

* Why is jextract also inferring platform endianness? Isn't the clang 
API powerful enough for that? I wonder what happens if an header is 
extracted in a VM that is running in a different environment.

Maurizio

On 02/02/2019 01:36, Henry Jen wrote:
> Updated webrev[1], the Value.Endianness class is basically same as java.nio.ByteOrder, they should probably be consolidated.
>
> The change is mainly to get rid of implicit endianness construction of Value instances, which is good to force considering of the correct endianness of the type. Removed INT16/32/64 from NativeTypes.
>
> Existing usage of INT16/32/64 are reviewed and
> - changed for platform endianness when the type is expected for native function, where are expected to be platform endian.
> - changed to LE version if it’s pure Java(where as long as same endianness is used, that should just work) or not relevant.
> - In some cases, changed to INT if native C type int is expected.
>
> [1] https://cr.openjdk.java.net/~henryjen/panama/endianness/05/webrev/
>
> Cheers,
> Henry
>
>
>> On Feb 1, 2019, at 2:12 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>> Yes, or even use LE types everywhere, since that's what we support right now anyway, and when we have better higher-level types we'll do the switch.
>>
>> Maurizio
>>
>> On 01/02/2019 02:24, Henry Jen wrote:
>>> So we are saying I revert back to have the
>>>
>>> final static INT16 = ByteOrder.naturalOrder() == ByteOrder.BIG_ENDIAN ? BE_INT16 : LE_INT16 in user’s code, that’s in EndiannessTest.java in this patch, and eventually all code where we are writing now to get JVM compatible INT16, INT32, INT64. Luckily we probably only need C types available now via SystemtemABI in most places.
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>>> On Jan 31, 2019, at 5:51 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>
>>>>
>>>> On 01/02/2019 01:45, Henry Jen wrote:
>>>>> We cannot just drop INT16, name it as PE_INT16 is fine. That’s the them of the discussion, we need something programmatically portable on JVM.
>>>> I would question the use of the term "need" here - longer term? Sure - but I can defo see the API not having platform-dependent layout types in the short term.
>>>>
>>>> In  other words, there are two parts to this problem; one is to take endianness into account when accessing memory, which you have done. The second is to provide a reasonable kit of starter layout types for people to use. These two problems don't have (and shouldn't) to be tackled at the same time.
>>>>
>>>> John suggested to go for all explicit mode - I'd say let's stick with that plan, and see what use cases will crop up in common code, and find ways (John's static import idea is a fine one - there could be others) to address them accordingly.
>>>>
>>>> Maurizio
>>>>
>>>>> Cheers,
>>>>> Henry
>>>>>
>>>>>
>>>>>> On Jan 31, 2019, at 5:13 PM, John Rose <john.r.rose at oracle.com> wrote:
>>>>>>
>>>>>> On Jan 31, 2019, at 5:00 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>>>> I'm saying make everything explicit - and drop INT16 at least for now, for either LE_INT16, or some other scheme.
>>>>>>>
>>>>>> That would be fine with me also, of course.