[foreign] RFR 8217664: jextract should check for error code returns from Type::getOffsetOf and Type::size

[Maurizio Cimadamore]

Binder changes and test look good!

Maurizio

On 24/01/2019 15:55, Jorn Vernee wrote:
> Maurizio Cimadamore schreef op 2019-01-24 15:44:
>> I'll leave the detailed review to Sundar - I just wanted to say that I
>> noticed this issues as well in the past (e.g. Clang API returning
>> weird things for incomplete arrays).
>>
>> On the binder test - maybe it shouldn't be in the jextract folder?
>> Also, while you test that allocation works, the test is basically
>> checking if two method calls return null as expected - which is a tad
>> weird? Shouldn't we also check that we can actually get an instance of
>> that array and that the size should be set to zero? (e.g. binder
>> should not crash when dereferencing one of these odd zero-length
>> arrays).
>
> Yes, good points - I just copied the functions being used by the 
> jextract test. I've moved the test files to test/jdk/java/foreign. I 
> fleshed out the test by setting/getting the fields of the struct, for 
> which I also had to do a small tweak to BoundedMemoryRegion.copyTo to 
> just return immediately if the length argument is 0.
>
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217664/webrev.01/
>
> Jorn
>
>> Maurizio
>>
>> On 24/01/2019 13:04, Jorn Vernee wrote:
>>> Hi,
>>>
>>> jextract uses libclang functions to query the size of a type and 
>>> offset of a field. The returned values are used for instance in 
>>> layout calculations. But, these functions can also return error 
>>> codes and there is currently no check for them. They are happily 
>>> being used in the layout calculations, and this can cause some 
>>> strange errors, or in some cases incorrect layouts are generated. To 
>>> give an example:
>>>
>>>     struct Foo {
>>>         int x;
>>>         void *list_of_data[];
>>>     };
>>>
>>> The expected layout of this struct (which uses an 
>>> incomplete/flexible array field) should be `[i32x32[0u64]]` but 
>>> jextract instead generates `[i32[0u64]x32]`. This particular case is 
>>> due to a bug in libclang where requesting the offset of a field in a 
>>> type with a flexible array member always returns an incomplete type 
>>> error, even though getting the offset of each fields should be 
>>> possible. This error code (-2) is then used in the layout 
>>> calculations and yields the incorrect result.
>>>
>>> So I've added checking to the returned value of Type::size and 
>>> Type::getOffsetOf and throwing an exception in case an error code is 
>>> returned. I'm also explicitly checking for the flexible array case 
>>> and throwing a UOE in that case. This also means I had to disable 
>>> the flexible array jextract test [1]. The test was passing, but at 
>>> least on Windows the wrong layout was being generated. Since we can 
>>> not reliably compute padding for types with flexible arrays using 
>>> the current method, I think it's better to throw an exception for 
>>> now until we can find a workaround for this bug in libclang, or the 
>>> bug gets fixed.
>>>
>>> Using a type with a flexible/incomplete array member in the binder 
>>> should still be fine of course, so I've added a separate test for that.
>>>
>>> Please review the following.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217664
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217664/webrev.00/
>>>
>>> Thanks,
>>> Jorn
>>>
>>> [1] : 
>>> http://hg.openjdk.java.net/panama/dev/file/c40a5a6dc24d/test/jdk/com/sun/tools/jextract/testStruct/struct.h#l85