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

Thu Mar 14 00:14:27 UTC 2019

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