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

[Maurizio Cimadamore]

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