[foreign] 8220296: Memory access should have layout with explicit endianness

[Henry Jen]

Looks good to me, given we have way to “infer” endianness, that what we really need. We moved ABI-dependent types into NativeTypes, that’s fine by me, just remember that when we add a new ABI, we may need to make changes to NativeTypes.java as well.

Otherwise, there is only one nit should be fixed, that is StructTest.java change below should be reverted, the C code is using int, not int32_t, so we should be using INT here.

Cheers,
Henry


--- old/test/jdk/com/sun/tools/jextract/testStruct/StructTest.java	2019-03-15 17:27:43.900730278 +0000
+++ new/test/jdk/com/sun/tools/jextract/testStruct/StructTest.java	2019-03-15 17:27:43.656723205 +0000
@@ -204,7 +204,7 @@
         assertTrue(layout instanceof Group);
         Group g = (Group) layout;
         System.out.println("Resolved layout   = " + g.toString());
-        assertEquals(g.bitsSize(), 8 * 4 * NativeTypes.INT.bytesSize());
+        assertEquals(g.bitsSize(), 8 * 4 * NativeTypes.INT32.bytesSize());
     }
 
     private void verifyGetAnonymous(Class<?> header) {



> On Mar 15, 2019, at 10:47 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> After some internal discussions, here's a revised patch:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8220296_v2/
> 
> This patch represents a U-turn in terms of what we have been working on so far. After pulling enough string off the proposal discussed in [1], we decided to retrace our steps and to abandon the tri-endianness model in favor ofa simpler model.
> 
> While having no-endianness is, by itself, a fine idea, when you combine that with API points which apply some degree of inference (to be usable), such as LayoutType::pointer, what you end up with is a model with _four_ (!!) endianness states: big, little, no, inferred.
> 
> On top of that, if all constants under NativeTypes are endianness-aware, it is impossible to compose constants and form more complex function types (as we don't want endianness in functions). This would force us to create yet another set of no-endianness constants, which seems, frankly, too much.
> 
> For these reason, we are reverting the implementation to the former, simpler state:
> 
> * lack of endianness in the API means inferred
> * conversely, all layout descriptions in annotations are fully explicit
> * layout descriptions support endianness via upper case, lower case letters as per [2]
> * significantly reworked NativeTypes - now the interface has explicit big endian/little endian layers, with ABI layers nested inside. But again, NativeTypes provide access to sensible defaults, where endianness and ABI are fixed by the platform on which we're running. This is in line with the overarching spirit described in the first bullet
> 
> I think this ends up being much saner to work with, as a client.
> 
> While I did not completely let go of the no-endian proposal, I think some extra thinking is needed in order to make that a viable option. One thing I liked about that proposal was the ability to define an endianness-agnostic layout, and then inject endianness after the fact. That said, I believe we can support a similar use case by means of a 'layout builder' interface, if we wanted to.
> 
> Cheers
> Maurizio
> 
> 
> On 08/03/2019 05:23, Henry Jen wrote:
>>> On Mar 7, 2019, at 4:14 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>> 
>>> Ok, I re-read your changeset, and I might have an idea of what happened:
>>> 
>>> You started off with adding the exception in BoundedPointer (yes!).
>>> 
>>> Some tests (a lot?) started to fail.
>>> 
>> You can give it a try, I might be biased, but I believe that when a type realized on the system, the endianness is determined, thus I have implemented that since beginning.
>> 
>> As I was starting, I tried allocate, cast and places to add endianness, but there are just too many scenarios where user need to give a LayoutType, and very likely that would be without an endianness, especially when often used NativeTypes without endianness.
>> 
>>> You retracted to a position where creating a LayoutType will inject some endianness into the layout.
>>> 
>>> I can see that this solves the test problem - but this doesn't help with the scenario I mentioned in the previous email (same code behaving differently in different archs) and I believe that renders the BoundedPointer check useless (since now all LayoutType acquire _some_ endianness, by construction).
>>> 
>> Like you said, any memory access should have endianness. I don’t know how to explain that when I say an int32 without endianness, it should be default to the host endianness. If I do know I want a specific endianness, then I will specify that.
>> 
>> I can certainly try to add some more constants like NativeTypes.HE_UNT32, but I thought we don’t want to go there based on earlier iteration.
>> 
>>> The checks start like this:
>>> 
>>>  if (type.layout() instanceof Value) {
>>> 
>>> Now, 'type' is a LayoutType - if a LayoutType's layout always gets some endianness injected (as per your patch), when would the check in BoundedPointer fail?
>>> 
>> Right, this is more serve as an assert to make sure we have explicit endianness.
>> 
>> Cheers,
>> Henry
>> 
>> 
>>> Maurizio
>>> 
>>> On 07/03/2019 23:58, Maurizio Cimadamore wrote:
>>>> I don't get what problem you are trying to address - can you please expand further, and maybe provide some examples?
>>>> 
>>>> To me, it looks like this is moving us away from the target we have set for ourselves.
>>>> 
>>>> 1) NE = not endianness (that is, endianness is an optional property of layouts)
>>>> 
>>>> 2) accessing memory w/o explicit endiannes is an error (note this is not even possible with byte buffer var handle, so there's a clear precedent)
>>>> 
>>>> 3) only place where NE is tolerated is functions, where in fact endianness is redundant
>>>> 
>>>> What you do on this patch is bad - because you can write the same code (e.g. read memory from NE layout and copy to a LE layout) and it will behave differently on different platforms, which is the very thing we want to avoid.
>>>> 
>>>> Maurizio
>>>> 
>>>> On 07/03/2019 19:24, Henry Jen wrote:
>>>>> Hi,
>>>>> 
>>>>> After spending quite some time to plug holes everywhere endianness need to be localized(allocation, cast, etc…), I think it’s best to be simple and consistent, and simply enforce LayoutType to have explicit endianness.
>>>>> 
>>>>> This is pretty close to what I came out at very beginning, any NE is really just the host endianness. However, in this edition, the “localize” of endianness happens in LayoutType, not Layout.
>>>>> 
>>>>> With that change, we enforce that function should not have byte-swapped layout.
>>>>> 
>>>>> The webrev[1] is up for review, let me know what you think.
>>>>> 
>>>>> Cheers,
>>>>> Henry
>>>>> 
>>>>> [1] http://cr.openjdk.java.net/~henryjen/panama/8220296/0/webrev/