[foreign] RFR 8220544: Jextract; library dependent header filtering
Jorn Vernee
jbvernee at xs4all.nl
Fri Mar 15 00:03:48 UTC 2019
Oh... I thought findEntry was added as part of Panama. I guess the
current difference in behavior between platforms exists in jdk/jdk as
well then.
Your patch works as well, I just thought that the VM code should try to
offer up uniform behavior, i.e. findEntry should always do a
non-recursive lookup on every platform. But, If the function is
pre-existing we should probably avoid changing the behavior.
I've tried with your patch as well, but 5 tests are failing with symbol
lookup errors. It seems to be a problem with the default library... i.e.
we should not do filtering when using the default library because the
library name will always be "<default>". I'm testing a fix.
Jorn
Maurizio Cimadamore schreef op 2019-03-15 00:27:
> Not super sure about this approach - findEntry was there before,
> aren't we changing the semantics of that function with your changes?
>
> This is in jdk/jdk (not Panama)
>
> http://hg.openjdk.java.net/jdk/jdk/file/8a66c92526cb/src/java.base/share/native/libjava/ClassLoader.c#l448
>
> That will most surely cause JNI link differences here
>
> http://hg.openjdk.java.net/jdk/jdk/file/8a66c92526cb/src/java.base/share/classes/java/lang/ClassLoader.java#l2690
>
> That was the reason why I put the changes inside the lookup() method.
>
> We could make a new lookup method for it, but I'd like first to
> understand why you think this filtering cannot be done in Java (since
> the functionality I'm using is supported on all platforms?)
>
> Maurizio
>
> On 14/03/2019 23:00, Jorn Vernee wrote:
>> FWIW, I've implemented the changes to findEntry based on your patch:
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.04/
>>
>> Jorn
>>
>> Jorn Vernee schreef op 2019-03-14 20:53:
>>> Sounds good!
>>>
>>> As for the implementation; since the filtering requirement is
>>> platform
>>> specific, it seems better to implement findEntry as a platform
>>> specific function in
>>> java.base/<platform>/native/libjava/jni_util_md.c
>>> (e.g. [1]) ?
>>>
>>> We already have a function there for findEntryInProcess. We could add
>>> one for findEntry as well, and then filter on unix.
>>>
>>> Jorn
>>>
>>> [1] :
>>> http://hg.openjdk.java.net/panama/dev/file/b3831a541894/src/java.base/unix/native/libjava/jni_util_md.c
>>> Maurizio Cimadamore schreef op 2019-03-14 20:37:
>>>> This is my take on the issue (see attached patch).
>>>>
>>>> Basically check path of library symbol against library path itself,
>>>> and if they don't match do not return the symbol via
>>>> Library::lookup.
>>>> This should provide stable semantics across platforms.
>>>>
>>>> Maurizio
>>>>
>>>> On 14/03/2019 19:10, Maurizio Cimadamore wrote:
>>>>> We do have soe support in the VM os class:
>>>>>
>>>>> // Locate DLL/DSO. On success, full path of the library is copied
>>>>> to
>>>>> // buf, and offset is optionally set to be the distance between
>>>>> addr
>>>>> // and the library's base address. On failure, buf[0] is set to
>>>>> '\0'
>>>>> // and offset is set to -1 (if offset is non-NULL).
>>>>> static bool dll_address_to_library_name(address addr, char* buf,
>>>>> int buflen, int* offset);
>>>>>
>>>>>
>>>>> trying to play with it and see what happens. Seems implemented for
>>>>> all OSs.
>>>>>
>>>>> Maurizio
>>>>>
>>>>> On 14/03/2019 18:58, Jorn Vernee wrote:
>>>>>> Here's the Windows prototype I had for iterating the symbols in a
>>>>>> library:
>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/itersyms/webrev.00
>>>>>>
>>>>>> Jorn
>>>>>>
>>>>>> Jorn Vernee schreef op 2019-03-14 19:50:
>>>>>>> Maurizio Cimadamore schreef op 2019-03-14 19:32:
>>>>>>>> So, I think that if we don't address this in some way, this work
>>>>>>>> is
>>>>>>>> not very effective on Linux/Mac, do yo agree?
>>>>>>>
>>>>>>> Yes, this is unfortunate...
>>>>>>>
>>>>>>>> As to how to address it, since there doesn't seem to be a way to
>>>>>>>> perform a 'local' library search, what if we augmented the
>>>>>>>> Symbol
>>>>>>>> interface to also return a Path (of the shared library the
>>>>>>>> symbol
>>>>>>>> belongs to) ?
>>>>>>>>
>>>>>>>> With that info we should be able to do some more filtering?
>>>>>>>>
>>>>>>>> To get the path, on *NIX we can use dladdr. Not sure if Win has
>>>>>>>> something similar.
>>>>>>>
>>>>>>> I ran into dladdr as well, but that seems to be a glibc
>>>>>>> extension? On
>>>>>>> Windows we have GetModuleFileName [1], so at least that's
>>>>>>> covered.
>>>>>>>
>>>>>>> But, it might be preferable to try and implement the symbol table
>>>>>>> crawl on the different operating systems, since that's more
>>>>>>> straight
>>>>>>> towards what we want to achieve. I mean, if we're going the
>>>>>>> non-portable route any ways... At least on Linux this seems to be
>>>>>>> possible as well [2] I want to say that this should be possible
>>>>>>> to do
>>>>>>> on most operating systems, as long as we can load the library
>>>>>>> file
>>>>>>> directly into memory, and know the file format.
>>>>>>>
>>>>>>> If we go that route I would need some help with the other
>>>>>>> platforms of course :)
>>>>>>>
>>>>>>> Jorn
>>>>>>>
>>>>>>> [1] :
>>>>>>> https://docs.microsoft.com/en-us/windows/desktop/api/libloaderapi/nf-libloaderapi-getmodulefilenamea
>>>>>>> [2] :
>>>>>>> https://stackoverflow.com/questions/26960377/how-to-programmatically-list-elf-shared-library-symbols
>>>>>>>> Maurizio
>>>>>>>>
>>>>>>>> On 14/03/2019 17:41, Maurizio Cimadamore wrote:
>>>>>>>>>
>>>>>>>>> On 14/03/2019 17:36, Jorn Vernee wrote:
>>>>>>>>>> I think you got it
>>>>>>>>>>
>>>>>>>>>> With dlsym it seems to be recursive: [1] "The search performed
>>>>>>>>>> by dlsym() is breadth first through the dependency tree of
>>>>>>>>>> these libraries."
>>>>>>>>>>
>>>>>>>>>> But, GetProcAdress on Windows [2] looks at the export table of
>>>>>>>>>> the library you give it, so AFAICT this is non-recursive.
>>>>>>>>>
>>>>>>>>> Yep - found the same, you beat me to it :-)
>>>>>>>>>
>>>>>>>>> Maurizio
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Jorn
>>>>>>>>>>
>>>>>>>>>> [1] : https://linux.die.net/man/3/dlsym
>>>>>>>>>> [2] :
>>>>>>>>>> https://docs.microsoft.com/en-us/windows/desktop/api/libloaderapi/nf-libloaderapi-getprocaddress
>>>>>>>>>>
>>>>>>>>>> Maurizio Cimadamore schreef op 2019-03-14 18:25:
>>>>>>>>>>> Nope - with this:
>>>>>>>>>>>
>>>>>>>>>>> $ cat foo.h
>>>>>>>>>>> #include <string.h>
>>>>>>>>>>>
>>>>>>>>>>> void foo(int f);
>>>>>>>>>>>
>>>>>>>>>>> $ cat foo.c
>>>>>>>>>>> #include "foo.h"
>>>>>>>>>>>
>>>>>>>>>>> void foo(int f) { }
>>>>>>>>>>>
>>>>>>>>>>> then,
>>>>>>>>>>>
>>>>>>>>>>> $ gcc -shared -o libfoo.so foo.c
>>>>>>>>>>>
>>>>>>>>>>> $ jextract -L . -l foo -d classes foo.h
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> And then
>>>>>>>>>>>
>>>>>>>>>>> classes
>>>>>>>>>>> ├── clang_support
>>>>>>>>>>> │ ├── stddef.class
>>>>>>>>>>> │ ├── stddef_h.class
>>>>>>>>>>> │ └── stddef$size_t.class
>>>>>>>>>>> ├── foo.class
>>>>>>>>>>> ├── foo_h.class
>>>>>>>>>>> ├── META-INF
>>>>>>>>>>> │ └── jextract.properties
>>>>>>>>>>> └── usr
>>>>>>>>>>> └── include
>>>>>>>>>>> ├── string.class
>>>>>>>>>>> ├── string_h.class
>>>>>>>>>>> ├── xlocale.class
>>>>>>>>>>> ├── xlocale_h.class
>>>>>>>>>>> ├── xlocale.class
>>>>>>>>>>> ├── xlocale.class
>>>>>>>>>>> └── xlocale.class
>>>>>>>>>>>
>>>>>>>>>>> 4 directories, 13 files
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I wonder if some of the library lookup functions we have on
>>>>>>>>>>> linux
>>>>>>>>>>> (dlopen and co.) are doing a 'recursive' search into
>>>>>>>>>>> dependent libs as
>>>>>>>>>>> well, which might be disabled on Win.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Maurizio
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 14/03/2019 16:38, Maurizio Cimadamore wrote:
>>>>>>>>>>>> Ah ok
>>>>>>>>>>>>
>>>>>>>>>>>> will give that another try
>>>>>>>>>>>>
>>>>>>>>>>>> Maurizio
>>>>>>>>>>>>
>>>>>>>>>>>> On 14/03/2019 16:16, Jorn Vernee wrote:
>>>>>>>>>>>>> Sorry, that's with the v3 patch
>>>>>>>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8220544/webrev.03/
>>>>>>>>>>>>> I reverted back after the concerns raised about changing
>>>>>>>>>>>>> the implementation of TypedefTree::def You're right in that
>>>>>>>>>>>>> this incorrectly includes some trees.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sorry,
>>>>>>>>>>>>> Jorn
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jorn Vernee schreef op 2019-03-14 17:08:
>>>>>>>>>>>>>> This produces test.class and test_h.class as expected, but
>>>>>>>>>>>>>> nothing else...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Used command is: `jextract -L . -l test test.h`
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> You would get stuff from string.h when the -l is omitted.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jorn
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maurizio Cimadamore schreef op 2019-03-14 17:00:
>>>>>>>>>>>>>>> I think you can reproduce simply by having an header like
>>>>>>>>>>>>>>> this:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> #include <string.h>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> void foo(int i);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if you extract that you would expect one symbol, but in
>>>>>>>>>>>>>>> reality you
>>>>>>>>>>>>>>> get also all the stuff from string.h.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Maurizio
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 14/03/2019 15:49, Jorn Vernee wrote:
>>>>>>>>>>>>>>>> The Windows jextract artifact for the OpenGL sample is a
>>>>>>>>>>>>>>>> different beast, because it relies on the standard
>>>>>>>>>>>>>>>> Windows OpenGL libraries. So, seeing some system headers
>>>>>>>>>>>>>>>> is to be expected :) In the case of python I'm also
>>>>>>>>>>>>>>>> seeing some system headers, because python depends on a
>>>>>>>>>>>>>>>> hand full of corecrt types, so that's also correct. But,
>>>>>>>>>>>>>>>> neither one seems to be referencing imaxabs, so I'm not
>>>>>>>>>>>>>>>> sure what's going on...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I'll try to see if I can find a repro example on Windows
>>>>>>>>>>>>>>>> to see about a fix. I'm not sure I see how the
>>>>>>>>>>>>>>>> SymbolFilter could get to the VM libraries? Though I
>>>>>>>>>>>>>>>> remember seeing a similar case in the past...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Jorn
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Maurizio Cimadamore schreef op 2019-03-14 15:55:
>>>>>>>>>>>>>>>>> I think I got at the bottom of this. I tried debugging
>>>>>>>>>>>>>>>>> an extraction
>>>>>>>>>>>>>>>>> of opengl, and it eventually ends up adding inttypes.h
>>>>>>>>>>>>>>>>> to the rootset
>>>>>>>>>>>>>>>>> because of this symbol:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> imaxabs
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The library filter doesn't work here, because this is a
>>>>>>>>>>>>>>>>> function
>>>>>>>>>>>>>>>>> defined in the set of default libraries that comes with
>>>>>>>>>>>>>>>>> the VM, so
>>>>>>>>>>>>>>>>> jextract 'thinks' this symbol is genuinely part of one
>>>>>>>>>>>>>>>>> of the
>>>>>>>>>>>>>>>>> libraries specified in the command line - except is
>>>>>>>>>>>>>>>>> not.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I tried an hacky fix which checks for absence of symbol
>>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>> Libraries.getDefaultLibrary - and that seems to give
>>>>>>>>>>>>>>>>> the expected
>>>>>>>>>>>>>>>>> results - but I leave this to your consideration.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Maurizio
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 14/03/2019 14:27, Maurizio Cimadamore wrote:
>>>>>>>>>>>>>>>>>> Hi Jorn,
>>>>>>>>>>>>>>>>>> I gave this a try with our OpenGL and Python examples.
>>>>>>>>>>>>>>>>>> The extraction process works fine, and the generated
>>>>>>>>>>>>>>>>>> jars are a bit smaller - but I still see a lot of
>>>>>>>>>>>>>>>>>> system-related headers. Python has still lots of them,
>>>>>>>>>>>>>>>>>> OpenGL less so, but even in the GL case, I think it's
>>>>>>>>>>>>>>>>>> interesting, because I can't see references to types
>>>>>>>>>>>>>>>>>> mentioned in these headers (nor in the resulting
>>>>>>>>>>>>>>>>>> classfiles). Do you know, on top of your head, as to
>>>>>>>>>>>>>>>>>> why this could be happening?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Maurizio
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 14/03/2019 12:45, Jorn Vernee wrote:
>>>>>>>>>>>>>>>>>>> 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