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

[Sundararajan Athijegannathan]

+1

-Sundar

On 30/01/19, 4:58 PM, Jorn Vernee wrote:
> 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