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

Jorn Vernee jbvernee at xs4all.nl
Thu Mar 14 12:45:13 UTC 2019 

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

Changed Treemaker implementation to recurse to the 'root' type when 
creating TypdefTree::def, and using that in DependencyFinder.

Jorn

Jorn Vernee schreef op 2019-03-14 12:49:
> Oh ok, sorry. I'll try that out.
> 
> Jorn
> 
> Maurizio Cimadamore schreef op 2019-03-14 12:43:
>> On 14/03/2019 11:40, Jorn Vernee wrote:
>>> I see what you mean.
>>> 
>>> Well, `struct BAR` has a definition cursor, but `struct BAR*` does 
>>> not have a definition/declaration, it's a type that's automatically 
>>> derived from `struct BAR`. So for `struct BAR*` there is no good 
>>> value for TypedefTree::def.
>> 
>> I know that there's no good value for struct BAR* - I was wondering if
>> the parser should detect the pointer, and recurse down, then store
>> 'struct BAR' into 'def'.
>> 
>> Maurizio
>> 
>>> 
>>> TypedefTree::def seems to be a helper method mostly targeted at 
>>> inline type definitions (looking at TypedefHandler [1]).
>>> 
>>> I think for my use case using TypedefTree.type().canonicalType() is 
>>> good enough, but I guess we could query that once in the parser and 
>>> then store it in a field in TypedefTree?
>>> 
>>> Jorn
>>> 
>>> [1] : 
>>> http://hg.openjdk.java.net/panama/dev/file/43ed53f40957/src/jdk.jextract/share/classes/com/sun/tools/jextract/TypedefHandler.java#l107
>>> 
>>> Maurizio Cimadamore schreef op 2019-03-14 11:57:
>>>> On 14/03/2019 00:23, Jorn Vernee wrote:
>>>>> 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.
>>>> 
>>>> Right - and I think what I'm claiming is that this is probably some
>>>> kind of a parser bug? That is, if Typedef::def (we should probably
>>>> find another name, 'def' is not good enough IMHO) is set correctly 
>>>> for
>>>> 
>>>> typedef struct BAR FOO;
>>>> 
>>>> it should also be set for
>>>> 
>>>> typedef struct BAR* FOO;
>>>> 
>>>> That is, I claim that from the typedef tree you should always see a
>>>> referenced type declaration w/o using the clang API.
>>>> 
>>>> Maurizio
>>>> 
>>>>> 
>>>>> 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

More information about the panama-dev mailing list