Re: RFR - Changes to address CCC 8014135: Support for statically linked agents
I updated the text in VirtualMachine.java. The new webrev is here: http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.01/ I ran it through javadoc and you can see the result here: http://cr.openjdk.java.net/~bpittore/8014135/javadoc/index.html Let me know if you have any comments. thanks, bill On 7/4/2013 12:41 PM, Alan Bateman wrote: On 03/07/2013 22:17, BILL PITTORE wrote: These changes address bug 8014135 which adds support for statically linked agents in the VM. This is a followup to the recent JNI spec changes that addressed statically linked JNI libraries( 8005716). The JEP for this change is the same JEP as the JNI changes: http://openjdk.java.net/jeps/178 Webrevs are here: http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/ I looked at the javadoc updates to the attach API. One thing that doesn't come across very clearly is the mapping from the agentLibrary parameter to agent-lib-name. I think that needs a few words so that it's clear that it is not literally looking for Agent_OnAttach_agent-lib-name but rather Agent_OnAttach + agent-lib-name where agent-lib-name is the library name in the agentLibrary parameter. As I recall, the JVM Tool Interface is supposed to be referred so as JVM TI rather than JVMTI. Otherwise it looks okay to me. -Alan
Re: RFR - Changes to address CCC 8014135: Support for statically linked agents
On 7/3/2013 6:32 PM, Jeremy Manson wrote: I know that this is mentioned in the JEP, but does it really make sense to have -agentpath work here, rather than just -agentlib? I would think that specifying a full pathname and getting the library loaded out of the launcher would be a little surprising to people who are basically saying please load this agent from the given path. Also, in practice, we do something very similar at Google, and, when testing, I find it occasionally useful to be able to say please load this agent on the command line via the agentpath I gave you, rather than loading the one with the same name I baked into the launcher. (FWIW, when we did this internally at Google, we added a special -XX flag for when the agent is in the launcher. I know that you don't want to go that way, though.) Thanks for the comments. I would say the thinking here is that we want this to be as transparent as possible to the end user. In our view of the end user is someone perhaps using an IDE and the actual execution of the VM with agent arguments is hidden. In your case it sounds like you are actually developing agents which is a whole different ball game. I could certainly see adding a -XX argument that forces agentpath to load the external library which would be good for agent development and debugging. No need to relink the entire VM with the new agent. Our tech lead is out on vacation this week so I'll bring this up when he's back. bill On Wed, Jul 3, 2013 at 2:17 PM, BILL PITTORE bill.pitt...@oracle.com mailto:bill.pitt...@oracle.com wrote: These changes address bug 8014135 which adds support for statically linked agents in the VM. This is a followup to the recent JNI spec changes that addressed statically linked JNI libraries( 8005716). The JEP for this change is the same JEP as the JNI changes: http://openjdk.java.net/jeps/178 Webrevs are here: http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/ http://cr.openjdk.java.net/%7Ebpittore/8014135/jdk/webrev.00/ http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/ http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/ The changes to jvmti.xml can also be seen in the output file that I placed here: http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/jvmti.html Thanks, bill
Re: RFR - Changes to address CCC 8014135: Support for statically linked agents
On 7/4/2013 12:41 PM, Alan Bateman wrote: On 03/07/2013 22:17, BILL PITTORE wrote: These changes address bug 8014135 which adds support for statically linked agents in the VM. This is a followup to the recent JNI spec changes that addressed statically linked JNI libraries( 8005716). The JEP for this change is the same JEP as the JNI changes: http://openjdk.java.net/jeps/178 Webrevs are here: http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/ I looked at the javadoc updates to the attach API. One thing that doesn't come across very clearly is the mapping from the agentLibrary parameter to agent-lib-name. I think that needs a few words so that it's clear that it is not literally looking for Agent_OnAttach_agent-lib-name but rather Agent_OnAttach + agent-lib-name where agent-lib-name is the library name in the agentLibrary parameter. As I recall, the JVM Tool Interface is supposed to be referred so as JVM TI rather than JVMTI. Ok, I'll try to re-word it and send it out again. bill Otherwise it looks okay to me. -Alan
RFR - Changes to address CCC 8014135: Support for statically linked agents
These changes address bug 8014135 which adds support for statically linked agents in the VM. This is a followup to the recent JNI spec changes that addressed statically linked JNI libraries( 8005716). The JEP for this change is the same JEP as the JNI changes: http://openjdk.java.net/jeps/178 Webrevs are here: http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/ http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/ The changes to jvmti.xml can also be seen in the output file that I placed here: http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html Thanks, bill
Re: Request for review fix for 8010668
Thanks for the review and thanks to Chris for pushing it. bill On 3/23/2013 7:15 AM, Alan Bateman wrote: On 22/03/2013 21:02, BILL PITTORE wrote: This fixes a problem with the recently committed changes for JNI static libraries. On an error condition or explicit unload request, statically linked libs should not be unloaded. http://cr.openjdk.java.net/~bpittore/8010668/webrev.00/ thanks, bill This looks good to me too, sorry we missed this when reviewing the original changes. -Alan.
Request for review fix for 8010668
This fixes a problem with the recently committed changes for JNI static libraries. On an error condition or explicit unload request, statically linked libs should not be unloaded. http://cr.openjdk.java.net/~bpittore/8010668/webrev.00/ thanks, bill
Re: Request for review- RFE 8005716
On 3/11/2013 9:40 AM, Alan Bateman wrote: On 08/03/2013 02:22, BILL PITTORE wrote: Moved the string allocation into buildJniFunctionName as Alan suggested. Built and tested on windows and linux. Updated the webrev: http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.02/ bill I see this updates the method descriptions to take on board Jeremy's comment on the possibility of the library being statically linked with the main executable with or without the VM. To be complete, I think this will require an update to the UnstatisfiedLinkError description too. Thanks for moving the sizing/allocation of the function name into buildJniFunctionName as that is cleaner and safer. Is FILENAME_MAX really the right limit to impose? Maybe JVM_MAXPATHLEN is the better choice. A minor point, but the buffer is overrsized by 1 for the shared library case, I think it should be: len = strlen(sym) + (cname != NULL ? strlen(cname)+1 : 0) + 1 The +2 covered both cases; but re-wrote it to be the more explicit version above. bill Otherwise, I think the implementation looks good to me. -Alan.
Re: Request for review- RFE 8005716
On 3/7/2013 1:18 PM, Jeremy Manson wrote: Hi guys, I'm really glad to see you are doing this. We've done something similar to good effect within Google (and we'll probably drop our similar, internal patch and pick up this patch once it is pushed). Have you thought about support for having a JNI_OnLoad inside of a main executable that *doesn't* have a postfixed _lib? Our Google-internal patch treats the main executable as a regular JNI library if you execute a special System.loadFromLauncher() function. We also do the same thing with JVMTI. Nit: The comments that read like this: * for example, L, and a native library called L is statically linked * with the VM, then the JNI_OnLoad_L function exported by the library Hows this sound. * If the filename argument, when stripped of any platform-specific library * prefix, path, and file extension, indicates a library whose name is, * for example, L, and a native library called L is statically linked * with the native application or the VM (which itself may be statically * linked into the native application), then the JNI_OnLoad_L * function exported by the library is invoked rather than attempting * to load a dynamic library. bill Should these read linked with the VM, or linked with the native application loading the VM? Jeremy On Wed, Mar 6, 2013 at 8:21 AM, Bob Vandette bob.vande...@oracle.com mailto:bob.vande...@oracle.com wrote: On Mar 5, 2013, at 7:36 PM, Dean Long wrote: If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on Windows, you can't just turn that into _JNI_OnLoad@8_ + libname. I think it needs to be _JNI_OnLoad_ + libname + @8 Good catch Dean. Looks like onLoadSymbols[] is unused in Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib(). Instead of adding getProcessHandle(), why not add JVM_FindBuiltinLibraryEntry() instead? This would make it easier to support table-lookup when runtime symbol information is missing or not supported by the platform. Bill has already factored out the implementation of getProcessHandle. Although your approach is cleaner, I'm concerned about creating a VM version dependency. For a traditional JRE that doesn't even require static library support, we'd have to make sure to run on a VM that support JNI_VERSION_1_8. It looks like the JDK maintains their own copy of jni.h. If they didn't do that, we would have already gone down that path. The jdk sources already contain several instances of dlopen, dlysym and the Windows equivalents so I don't see a need to add a new VM function. Bob. dl On 3/5/2013 3:05 PM, bill.pitt...@oracle.com mailto:bill.pitt...@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/%7Ebpittore/8005716/jdk-webrev.00/ http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/ http://cr.openjdk.java.net/%7Ebpittore/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. thanks, bill
Re: Request for review- RFE 8005716
On 3/7/2013 3:50 PM, Alan Bateman wrote: On 07/03/2013 16: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@8 to _JNI_OnLoad_libname@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 Thanks for sorting out the decorator issue. One comment on that part of the implementation is that findJniFunction is sizing the buffer and buildJniFunctionName is using strcpy in the components. I wonder if this could be changed so that buildJniFunctionName sizes the buffer and uses strncpy. That way it will be obviously safe when just looking at one function. It makes it easier for static analysis tools to check too. I actually was passing in 'len' at one point and checking but it seemed redundant. The comment about static analysis tools having a fit makes sense. I'll tweak it as you suggest. bill -Alan
Re: Request for review- RFE 8005716
Moved the string allocation into buildJniFunctionName as Alan suggested. Built and tested on windows and linux. Updated the webrev: http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.02/ bill On 3/7/2013 3:50 PM, Alan Bateman wrote: On 07/03/2013 16: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@8 to _JNI_OnLoad_libname@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 Thanks for sorting out the decorator issue. One comment on that part of the implementation is that findJniFunction is sizing the buffer and buildJniFunctionName is using strcpy in the components. I wonder if this could be changed so that buildJniFunctionName sizes the buffer and uses strncpy. That way it will be obviously safe when just looking at one function. It makes it easier for static analysis tools to check too. -Alan
Re: Request for review- RFE 8005716
Actually for windows I *did* export the undecorated name. I just didn't see where I did it in the VS IDE. If you don't export the undecorated name it doesn't work of course. bill On 3/5/2013 11:13 PM, BILL PITTORE wrote: On 3/5/2013 7:36 PM, Dean Long wrote: If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on Windows, you can't just turn that into _JNI_OnLoad@8_ + libname. I think it needs to be _JNI_OnLoad_ + libname + @8 I'll look into that. When I built for windows and ran our test, the JNI_OnLoad_TestStaticLib was exported without the decoration just based on the JNIEXPORT JNICALL specifiers on the function. I didn't do anything special to export it. But I recall this problem from another project. 10 1014 JNI_OnLoad_TestStaticLib = @ILT+15(?JNI_OnLoad_TestStaticLib@@YGHPAUJavaVM_@@PAX@Z) Looks like onLoadSymbols[] is unused in Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib(). I'll remove that. I redid the call to findBuiltinJniFunction but forgot to remove that. Instead of adding getProcessHandle(), why not add JVM_FindBuiltinLibraryEntry() instead? This would make it easier to support table-lookup when runtime symbol information is missing or not supported by the platform. Not sure I'm following you on this. Make JVM_FindBuiltinLibraryEntry() an exported function in the VM? How does it find JNI_OnLoad_L? Via a table setup by the developer/build system when the library is linked in? bill dl On 3/5/2013 3:05 PM, bill.pitt...@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. thanks, bill
Re: Request for review- RFE 8005716
On 3/6/2013 12:50 PM, Mike Duigou wrote:
Hi Bill;
Some notes from reviewing the JDK side changes.
System.java/Runtime.java:
The example which begins with the name If the filename argument, needs to better
identify that L is an example. (Italics?)
Re-worded that a bit.
java/lang/ClassLoader.java:
NativeLibrary::fromClass could be final.
Ok.
ClassLoader.c:
In Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib()
These two values are known at compile time.
int prefixLen = (int) strlen(JNI_LIB_PREFIX);
int suffixLen = (int) strlen(JNI_LIB_SUFFIX);
Some of the error conditions don't throw exceptions. Such as:
if (cname == NULL) {
return NULL;
}
Fixed.
The prefix and suffix are stripped from cname without checking that cname
actually contains the prefix or suffix.
..._findBuiltinLib is only called after System.mapLibraryName has added
prefix and suffix.
if (len prefixLen) is invariant.
Based on System.mapLibraryName always called, then this could just be an
assert. Unless JNU_GetStringPlatformChars returns some bogus string.
src/solaris/native/common/jni_util_md.c:
void* getProcessHandle() {
static void* procHandle = NULL;
if (procHandle == NULL) {
procHandle = (void*)dlopen(NULL, RTLD_LAZY);
}
return procHandle;
}
Why is the error handling code commented out?
That was just for some debugging. I've removed it.
Thanks for commenting. I'll get a new webrev up shortly.
bill
Mike
On Mar 5 2013, at 15:05 , bill.pitt...@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.
thanks,
bill
Request for review- RFE 8005716
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. thanks, bill
Re: Request for review- RFE 8005716
On 3/5/2013 7:36 PM, Dean Long wrote: If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on Windows, you can't just turn that into _JNI_OnLoad@8_ + libname. I think it needs to be _JNI_OnLoad_ + libname + @8 I'll look into that. When I built for windows and ran our test, the JNI_OnLoad_TestStaticLib was exported without the decoration just based on the JNIEXPORT JNICALL specifiers on the function. I didn't do anything special to export it. But I recall this problem from another project. 10 1014 JNI_OnLoad_TestStaticLib = @ILT+15(?JNI_OnLoad_TestStaticLib@@YGHPAUJavaVM_@@PAX@Z) Looks like onLoadSymbols[] is unused in Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib(). I'll remove that. I redid the call to findBuiltinJniFunction but forgot to remove that. Instead of adding getProcessHandle(), why not add JVM_FindBuiltinLibraryEntry() instead? This would make it easier to support table-lookup when runtime symbol information is missing or not supported by the platform. Not sure I'm following you on this. Make JVM_FindBuiltinLibraryEntry() an exported function in the VM? How does it find JNI_OnLoad_L? Via a table setup by the developer/build system when the library is linked in? bill dl On 3/5/2013 3:05 PM, bill.pitt...@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. thanks, bill
RFR- 7200297 jdwp and hprof code do not handle multiple sun.boot.library.path elements correctly
This bug was reviewed by serviceability and hotspot-runtime-dev lists since it dealt with jvmti agent code. Comments welcome. The intent is to push into tl/jdk tree and then eventually back into jdk7u-dev hopefully. Webrev here: http://cr.openjdk.java.net/~bpittore/7200297/webrev.04/ And previous reviews here: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2012-November/004801.html thanks, bill
Re: RFR- 7200297 jdwp and hprof code do not handle multiple sun.boot.library.path elements correctly
Ok, Thanks. David Holmes asked that I send to this alias and he would re-review then push it for me. bill | On 11/30/2012 10:37 AM, Alan Bateman wrote: | On 30/11/2012 15:16, BILL PITTORE wrote: This bug was reviewed by serviceability and hotspot-runtime-dev lists since it dealt with jvmti agent code. Comments welcome. The intent is to push into tl/jdk tree and then eventually back into jdk7u-dev hopefully. Webrev here: http://cr.openjdk.java.net/~bpittore/7200297/webrev.04/ And previous reviews here: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2012-November/004801.html thanks, bill Bill - if you already have a reviewer then just go ahead and push the fix, you don't need to get an additional review here. -Alan
RFR- 7200297 jdwp and hprof code do not handle multiple sun.boot.library.path elements correctly
This bug was reviewed by serviceability and hotspot-runtime-dev lists since it dealt with jvmti agent code. Comments welcome. The intent is to push into tl/jdk tree and then eventually back into jdk7u-dev hopefully. Webrev here: http://cr.openjdk.java.net/~bpittore/7200297/webrev.04/ And previous reviews here: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2012-November/004801.html thanks, bill
