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

[Sundararajan Athijegannathan]

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