[foreign] RFR 8211060: Library.getDefault() does not work on Windows

[Jorn Vernee]

Ok, then I think we agree on dropping the default library, and the 
question becomes how to handle the current tests now having to load 
different libraries on different platforms.

For the most part the Java side is completely the same, only the value 
being passed to `NativeHeader#libraries` would have to change depending 
on the platform.

Any suggestions about how to go about that? My only idea of just 
copy-pasting every test, adding a @requires tag and then changing just 
that string seems like overkill.

Jorn

Sundararajan Athijegannathan schreef op 2018-09-24 14:22:
> Also it seems EnumProcessModules is meant for debuggers [1]. I agree
> that we should probably not have the notion of the default library.
> 
> [1]
> https://docs.microsoft.com/en-us/windows/desktop/api/psapi/nf-psapi-enumprocessmodules
> 
> -Sundar
> 
> On 24/09/18, 5:02 PM, Jorn Vernee wrote:
>> I also think the approach of removing the default library is more 
>> robust, since a symbol can be loaded from several libraries at once, 
>> but when using the default library you don't know which one you're 
>> gonna get. I can imagine a big project using several decencies which 
>> try to load the same symbol from different libraries through the 
>> default library and things breaking when the wrong function address is 
>> loaded.
>> 
>> On the other hand, library names for the standard library are platform 
>> specific. e.g. the standard C library dll is named msvcrt on windows, 
>> so writing platform agnostic code that just depends on the portable 
>> standard library, like the tests we have, would be more difficult.
>> 
>> Maybe that problem could be solved by shipping pre-computed library 
>> interfaces for the standard library with the JDK, where the value of 
>> NativeHeader#libraries would be platform specific. That would also 
>> decrease the time-to-helloworld for foreign, and I think that at least 
>> the standard C library is small enough that that won't be problematic.
>> 
>> Jorn
>> 
>> Maurizio Cimadamore schreef op 2018-09-24 13:10:
>>> Hi Jorn,
>>> thanks for the patch. As mentioned last time we have two options 
>>> here:
>>> one is to follow the approach forward in your patch; another would be
>>> to ditch Library.getDefault() entirely and adopt a more explicit
>>> approach - that is to always require 'implicit' libraries to be
>>> mentioned - either by jextract artifacts or by API points.
>>> 
>>> A note on the latter - when you do:
>>> 
>>> Libraries.bindRaw(lookup, Foo.class)
>>> 
>>> the code lookup the @NativeHeader annotation on Foo.class and tries 
>>> to
>>> extract required library names from there. Currently, we do not add
>>> library names for standard libraries such as "c" or "m" (math), which
>>> is what led us down the (slippery?) slope of having a
>>> Library.getDefault somewhere.
>>> 
>>> Another option would be to have jextract to always generate the names
>>> of the libraries an artifact depends on, and then the API should 
>>> throw
>>> an exception if a @NativeHeader is found with no libraries. More
>>> specifically, jextract should always add "c" to the set of used
>>> libraries in the @NativeHeader annotation (other libraries can be
>>> explicitly supplied using the -l flag).
>>> 
>>> Note that I'm not saying "the binder should always add in 'clib'" for
>>> you, because that's kind of a lame assumption: it works obviously 
>>> well
>>> for C but it doesn't make a lot of sense if you want to use Panama 
>>> for
>>> other purposes/languages. Which seems to suggest that, as far as the
>>> binder is concerned, library dependencies should always be explicit.
>>> 
>>> Thoughts?
>>> 
>>> Maurizio
>>> 
>>> 
>>> 
>>> On 24/09/18 11:52, Jorn Vernee wrote:
>>>> Hello,
>>>> 
>>>> I'd like to contribute a patch that adds support for the default 
>>>> library on windows.
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211060
>>>> Diff: 
>>>> https://gist.github.com/JornVernee/7d45780df082cbfb27417b437c7b13a8
>>>> 
>>>> As mentioned before [1], this fixes 2 bugs:
>>>> 
>>>> 1.) When no library was loaded 
>>>> ClassLoader#NativeLibrary#getFromClass
>>>> threw an NPE (at least on windows). That is fixed by returning
>>>> defaultLibrary.fromClass when the nativeLibraryContext is empty.
>>>> 
>>>> 2.) The default library search was not working on windows. It was 
>>>> using
>>>> a default handle, which works on unix (dlsym(RTLD_DEFAULT)), but not 
>>>> on
>>>> windows (see https://stackoverflow.com/q/23437007). I have changed 
>>>> the
>>>> implementation from using a default handle to using a new native
>>>> function findEntryInProcess, which does the right thing for Windows
>>>> (iterate over all loaded modules), and does the same thing it used 
>>>> to
>>>> for unix. findEntry is now a Java method, and the original findEntry 
>>>> is
>>>> renamed to findEntry0. The NativeLibrary implementation of findEntry
>>>> forwards to findEntry0, and the anonymous class of the default 
>>>> library
>>>> overrides to forward to findEntryInProcess.
>>>> 
>>>> Please test this patch on unix as well, since I don't have the 
>>>> ability to do so.
>>>> 
>>>> Jorn
>>>> 
>>>> [1] : 
>>>> http://mail.openjdk.java.net/pipermail/panama-dev/2018-September/002644.html