[foreign] RFR 8217781: Filter child cursors when doing recursive offset lookup

[Jorn Vernee]

Thanks,

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217781/webrev.02/

I also split the exception message I added in RecordLayoutComputer since 
that was also pretty long.

Jorn

Sundararajan Athijegannathan schreef op 2019-01-30 04:58:
> +1.
> 
> Good fix! subtle bug.
> 
> Minor: Cursor.toString is a very long line. Perhaps it can folded into 
> 2 lines.
> 
> -Sundar
> 
> On 30/01/19, 3:08 AM, Maurizio Cimadamore wrote:
>> Looks good - Sundar please also take a look
>> 
>> Cheers
>> Maurizio
>> 
>> On 25/01/2019 19:45, Jorn Vernee wrote:
>>>> But then, as I was writing this, I realized that 
>>>> RecordLayoutComputer
>>>> seems to have several helper methods which take a Cursor as input,
>>>> when in reality the only input we ever pass is the record cursor
>>>> itself (which is already stashed in a field) - so maybe we should 
>>>> get
>>>> rid of these extra Cursor params and use the field?
>>> 
>>> This is not quite possible. `hasIncompleteArray` also calls these 
>>> methods with the child cursors of anonymous nested structs and 
>>> unions. So we can't just make the helper methods non-static and use 
>>> the field.
>>> 
>>> It's quite unfortunate the way the bug works. We have to check for 
>>> incomplete array fields before trying to process _any_ of the fields, 
>>> which means doing a recursive lookup into the anonymous structs and 
>>> unions inside the type.
>>> 
>>> But there is a simplification possible by replacing isFlattenable 
>>> with the method you suggest (which I've named `flattenableChildren` 
>>> to show the difference with `children()`)
>>> 
>>> Updated webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217781/webrev.01/
>>> 
>>> Cheer,
>>> Jorn
>>> 
>>> Maurizio Cimadamore schreef op 2019-01-25 19:40:
>>>> Looks good,
>>>> Some minor suggestions - these kind of issues arise because the code
>>>> should never call cursor.children().
>>>> 
>>>> I suggest getting rid of isFlattenable, and have an accessor which 
>>>> does this:
>>>> 
>>>> Stream<Cursor> children(Cursor c) {
>>>>    return c.children()
>>>>                    .filter(RecordLayoutComputer::isFlattenable)
>>>> }
>>>> 
>>>> But then, as I was writing this, I realized that 
>>>> RecordLayoutComputer
>>>> seems to have several helper methods which take a Cursor as input,
>>>> when in reality the only input we ever pass is the record cursor
>>>> itself (which is already stashed in a field) - so maybe we should 
>>>> get
>>>> rid of these extra Cursor params and use the field?
>>>> 
>>>> Maurizio
>>>> 
>>>> On 25/01/2019 12:26, Jorn Vernee wrote:
>>>>> Hi,
>>>>> 
>>>>> Based on JDK-8217664 [1], I have found the following bug with this 
>>>>> layout:
>>>>> 
>>>>> ```
>>>>> struct Foo {
>>>>>     struct {
>>>>>         union {
>>>>>             int x;
>>>>>         } Bar;
>>>>>     };
>>>>> };
>>>>> ```
>>>>> 
>>>>> To find the offset of the anonymous struct in Foo, we have to find 
>>>>> the offset of it's first field. But, the current code is looking 
>>>>> for the first child _cursor_ of the anonymous struct, which is the 
>>>>> union declaration, and not the field 'Bar'. This again tries to 
>>>>> look up the first field of 'Bar' which is 'x', so we end up looking 
>>>>> up the field 'x' in Foo, which throws an error.
>>>>> 
>>>>> The fix is simple, we just have to filter for anonymous 
>>>>> struct/union and FieldDecl child cursors when doing a recursive 
>>>>> offset lookup. I've also added an exception message to the 
>>>>> recursive lookup, which helped debug this.
>>>>> 
>>>>> Please review the following.
>>>>> 
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217781
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217781/webrev.00/
>>>>> 
>>>>> Thanks,
>>>>> Jorn
>>>>> 
>>>>> [1] : https://bugs.openjdk.java.net/browse/JDK-8217664