[foreign] RFR 8220544: Jextract; library dependent header filtering
Jorn Vernee
jbvernee at xs4all.nl
Thu Mar 14 11:40:32 UTC 2019
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.
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