Request for review- RFE 8005716

[Mike Duigou]

Changes look good to me.

Mike
On Mar 7 2013, at 08:36 , BILL PITTORE wrote:

> I updated the code based on the feedback. To fix the windows symbol name issue that Dean brought up I added a platform specific function, buildJniFunctionName. In windows it will properly convert _JNI_OnLoad at 8 to _JNI_OnLoad_libname at 8 as well as handle JNI_OnLoad symbol name.
> 
> Fixed copyright headers as well.
> 
> Tested on linux and windows
> 
> Webrevs are here:
> 
> http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.01/
> http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/
> 
> bill
> 
> On 3/6/2013 8:47 AM, Alan Bateman wrote:
>> On 05/03/2013 23:05, bill.pittore at oracle.com wrote:
>>> This request is tied to bugid 8005716 that deals with adding support for statically linked JNI libraries.
>>> 
>>> The JEP is here: http://openjdk.java.net/jeps/178
>>> 
>>> The bug is here:http://bugs.sun.com/view_bug.do?bug_id=8005716
>>> 
>>> The webrevs are here:
>>> 
>>> http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.00/
>>> http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/
>>> 
>>> The main piece of functionality is to check for the presence of JNI_OnLoad_libname to determine if the library specified by 'libname' has been statically linked into the VM. If the symbol is found, it is assumed that the library is linked in and will not be dynamically loaded.
>> Overall I think this is quite good and follows the proposal in the JEP. I don't see anything obvious wrong with the logic and everything should just work for shared libraries as it does today. I assume you'll run all the appropriate tests.
>> 
>> I see Dean's suggestion to add a JVM function to allow for a lookup table when the symbol information is available, If you do that then you'll need to get the hotspot changes in first. Alternatively, change what you have later once the hotspot changes are in.Just on the
>> 
>> As findBuiltJniFunction can locate a built-in or a JNI_OnLoad/OnUnload in a library then the function name is probably not quite right (shouldn't have "Builtin" in the name).
>> 
>> A minor nit in _findBuiltinLib is that if the OOME path should call JNU_ReleaseStringPlatformChars before returning.
>> 
>> There are a few commented out statements in jni_util_md.c that I assume will be removed. Also you might want to check the indentation in both getProcessHandle implementations as they look like 2-spaces whereas the libs uses 4 (although this may be mute if you use a JVM function).
>> 
>> Otherwise I think this is good and I can sponsor the jdk part to this and help get it into jdk8/tl.
>> 
>> -Alan
>> 
> 
> 
>