[foreign] RFR 8220544: Jextract; library dependent header filtering

Thu Mar 14 10:35:57 UTC 2019

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.03/

Removed the getTypedefDeclUnderlyingType() binding, now adding just 
Type::canonicalType() to the root set.

Jorn

Jorn Vernee schreef op 2019-03-14 01:23:
> Sorry, bad example. The actual failure case is:
> 
>      struct BAR { int x; }
> 
>      typedef struct BAR* FOO;
> 
> The TreeMaker code looks at the canonical type of the typedef `struct
> BAR*` which is neither a type definition or declaration, so
> TypedefTree::def is empty. But, in the case of dependecy analysis, we
> still want to inspect that type, get the pointee type and add it to
> the root set.
> 
> Jorn
> 
> Jorn Vernee schreef op 2019-03-14 01:14:
>> Maurizio Cimadamore schreef op 2019-03-14 01:04:
>>> So, if I understand correctly, the issue is that if you have:
>>> 
>>> typedef struct { int x; } FOO;
>>> 
>>> you want the struct to be added to the root set. Which make sense.
>>> 
>>> Now, with respect to getUnderlyingType - I'm a bit at odds; it seems
>>> to me that the jextract tree should have the right info, under
>>> TypedefTree::def, at least that is set by the parser for
>>> declarations/definitions.
>> 
>> That exact case is covered by TypedefTree::def, but it doesn't cover a
>> case like this:
>> 
>>     struct BAR { int x; }
>> 
>>     typedef struct BAR FOO;
>> 
>> i.e. TypedefTree::def is only set when the typedef contains an inline
>> definition or a prototype. But, if the type is just referenced,
>> TypedefTree::def is an empty Optional.
>> 
>>> Wouldn't visiting TypedefTree::def (if present) be enough (and more 
>>> in
>>> sync with the rest of jextract) ? After all, that's the kind of thing
>>> you wanna be sure not to miss by the dependency analysis -
>>> getUnderlyingType seems a tad too noisy?
>> 
>> This is what I had before, but it fell short in some cases.
>> 
>>> In other words, let's say that there's a typedef in an header of a
>>> library you want to jextract which is related, via a chain of other 
>>> 42
>>> typedefs to something in some obscure system library. Are you sure 
>>> you
>>> want the resulting artifact to have all the 42 intermediate typedefs?
>>> After all, the library you wanna extract has just defined 'its
>>> version' of the name for that particular construct... so it's very
>>> likely that users of that library will prefer that name to the other
>>> 42 ones?
>> 
>> 42 intermediate typedefs seems a bit extreme...
>> 
>> But, I think you have a point. Putting the typedef and the canonical
>> type into the root set is probably enough.
>> 
>> Jorn
>> 
>>> Maurizio
>>> 
>>> On 13/03/2019 23:31, Jorn Vernee wrote:
>>>> Update webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.02/
>>>> 
>>>> This fixes an issue where typedef underlying types were not being 
>>>> added to the root set.
>>>> 
>>>> It turns out I was using the wrong API to get the underlying type, 
>>>> and the needed API didn't have a binding yet :)
>>>> 
>>>> The changes with respect to the previous webrev;
>>>>   * added Cursor.getTypedefDeclUnderlyingType() binding
>>>>   * added TypdefTree.underlyingType() method
>>>>   * changed DependencyFinder to use this method instead of 
>>>> typeDefinition()
>>>>   * added test case for the failure case (also simplified test class 
>>>> to only run jextract once and reuse the result)
>>>> 
>>>> Cheers,
>>>> Jorn
>>>> 
>>>> Maurizio Cimadamore schreef op 2019-03-13 22:43:
>>>>> On 13/03/2019 21:36, Jorn Vernee wrote:
>>>>>>> Good questions. When no filter patterns are specified that is the 
>>>>>>> case
>>>>>>> everything is outputted, but when filter patterns are specified
>>>>>>> headers are only added to the root set if they contain a symbol 
>>>>>>> that
>>>>>>> passes the filter.
>>>>>> 
>>>>>> FWIW, basically headers with symbols that pass both types of 
>>>>>> filter are added to the root set. There's 1.) Filtereing based on 
>>>>>> whether a symbol is found in a library, and 2.) based on the used 
>>>>>> filter patterns (--include-symbols & --exclude-symbols). I thought 
>>>>>> it would be better to exclude a header from the root set if all 
>>>>>> the symbols in it were filtered out by the filter patterns.
>>>>> 
>>>>> Yep - I think that sounds sensible
>>>>> 
>>>>> Maurizio
>>>>> 
>>>>>> 
>>>>>> Jorn
>>>>>> 
>>>>>> Jorn Vernee schreef op 2019-03-13 22:30:
>>>>>>> Maurizio Cimadamore schreef op 2019-03-13 22:08:
>>>>>>>> Hi Jorn,
>>>>>>>> this looks very good indeed. I'd like to play with it a bit 
>>>>>>>> before we
>>>>>>>> go ahead with this, if that's ok.
>>>>>>> 
>>>>>>> Please do!
>>>>>>> 
>>>>>>>> Also, one question: if the user does not specify "-l" what 
>>>>>>>> happens? I
>>>>>>>> guess the root set will then contain all the headers and we just 
>>>>>>>> get
>>>>>>>> back the full output (which would be ok) ?
>>>>>>> 
>>>>>>> Good questions. When no filter patterns are specified that is the 
>>>>>>> case
>>>>>>> everything is outputted, but when filter patterns are specified
>>>>>>> headers are only added to the root set if they contain a symbol 
>>>>>>> that
>>>>>>> passes the filter.
>>>>>>> 
>>>>>>> Maybe we should just fall back to 'include-everything' when no 
>>>>>>> "-l"
>>>>>>> options are specified, regardless of filter?
>>>>>>> 
>>>>>>> Jorn
>>>>>>> 
>>>>>>>> Maurizio
>>>>>>>> 
>>>>>>>> On 13/03/2019 18:06, Jorn Vernee wrote:
>>>>>>>>> Updated webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.01/ 
>>>>>>>>> Fixed the bug, also added tests & ran all the examples again to 
>>>>>>>>> try and find other bugs (nothing found).
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Jorn
>>>>>>>>> 
>>>>>>>>> Jorn Vernee schreef op 2019-03-13 17:50:
>>>>>>>>>> Discovered a bug in this after posting :(
>>>>>>>>>> 
>>>>>>>>>> DependencyFinder also needs to look for pointee and array 
>>>>>>>>>> element type
>>>>>>>>>> cursors (which it currently doesn't). I've tested a working 
>>>>>>>>>> fix, but
>>>>>>>>>> will also add some tests for the particular cases. Updated 
>>>>>>>>>> webrev
>>>>>>>>>> coming soon™
>>>>>>>>>> 
>>>>>>>>>> Jorn
>>>>>>>>>> 
>>>>>>>>>> Jorn Vernee schreef op 2019-03-13 17:21:
>>>>>>>>>>> Hi,
>>>>>>>>>>> 
>>>>>>>>>>> After some discussion:
>>>>>>>>>>> https://mail.openjdk.java.net/pipermail/panama-dev/2019-March/004828.html 
>>>>>>>>>>> I've implemented the discussed approach to header & 
>>>>>>>>>>> declaration filtering.
>>>>>>>>>>> 
>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8220544
>>>>>>>>>>> Webrev: 
>>>>>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.00/ 
>>>>>>>>>>> There are a few new implementation classes:
>>>>>>>>>>>   * PattertnFilter; a helper class that encapsulates the 
>>>>>>>>>>> Pattern based
>>>>>>>>>>> filtering code previously found in SymbolFilter. This code 
>>>>>>>>>>> had to be
>>>>>>>>>>> reused.
>>>>>>>>>>>   * Filters; a Context sub-component holding PatternFilters 
>>>>>>>>>>> for
>>>>>>>>>>> different types of declarations.
>>>>>>>>>>>   * DependencyFinder; finds dependencies of the root set of
>>>>>>>>>>> declarations found in the root headers.
>>>>>>>>>>>   * ElementFilter; filters out non-library symbol elements 
>>>>>>>>>>> based on
>>>>>>>>>>> whether they appear in a root header or are required by 
>>>>>>>>>>> something
>>>>>>>>>>> therein.
>>>>>>>>>>>   * RootSet; a helper class for managing root headers + 
>>>>>>>>>>> required element Trees.
>>>>>>>>>>> 
>>>>>>>>>>> I also encountered some crashes when debug printing Trees, 
>>>>>>>>>>> which I've
>>>>>>>>>>> fixed/worked around accordingly.
>>>>>>>>>>> 
>>>>>>>>>>> FWIW, I tried this out with the Windows registry API and it's 
>>>>>>>>>>> working
>>>>>>>>>>> very nicely! Going from well over 100 headers to under 10. 
>>>>>>>>>>> Some room
>>>>>>>>>>> for improvement still exists; some empty headers and static 
>>>>>>>>>>> forwarders
>>>>>>>>>>> could still be omitted, but imho that should be handled by a 
>>>>>>>>>>> separate
>>>>>>>>>>> patch.
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jorn