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

[Jorn Vernee]

> 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