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

[Jorn Vernee]

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