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

[Sundararajan Athijegannathan]

Looks good

PS. I did not re-run the tests with this latest patch though

-Sundar

On 25/01/19, 4:59 PM, Jorn Vernee wrote:
> Hi Sundar,
>
> I have removed the unused code. I've also added a test for the various 
> flexible/incomplete array cases that should be caught 
> (jextract/incompleteArrays/IncompleteArrayTest.java), can you take a 
> look?
>
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217664/webrev.04/
>
> Thanks,
> Jorn
>
> Sundararajan Athijegannathan schreef op 2019-01-25 09:57:
>> jextract/clang changes fine. Tests pass on Mac. But as I mentioned,
>> please remove unused code in tests rather than commenting it.
>>
>> -Sundar
>>
>> On 25/01/19, 7:51 AM, Sundararajan Athijegannathan wrote:
>>> Unused code should be removed and not just commented (struct.h for 
>>> eg). We've history in the repo and we can always go back.
>>>
>>> PS. I'll build/test on Mac and respond.
>>>
>>> -Sundar
>>>
>>> On 25/01/19, 1:10 AM, Jorn Vernee wrote:
>>>> Updated webrev for recent push: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217664/webrev.03/
>>>>
>>>> Jorn
>>>>
>>>> Jorn Vernee schreef op 2019-01-24 14:04:
>>>>> 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 
>>>>>