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

[Sundararajan Athijegannathan]

+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