On Aug 2, 2013, at 11:11 PM, Bill Pittore wrote: > On 8/2/2013 9:12 PM, serguei.spit...@oracle.com wrote: >> Hi Bill, >> >> A couple of more questions. >> >> webrev.01/jvmti.html >> >> An agent L whose image has been combined with the VM *is defined* as >> /statically linked/ >> if and only if the agent exports a function called Agent_OnLoad_L. >> >> A question to the above. >> Are we going to allow to link a library dynamically if it exports both >> the Agent_OnLoad and Agent_OnLoad_L functions? >> It can be convenient if a library exports both Agent_OnLoad and >> Agent_OnLoad_L >> as it can be linked statically or dynamically depending on the need without >> changes. >> > It would be nice but the problem is that you could only link one agent into > the VM if it exported Agent_OnLoad. Otherwise there would be a symbol > collision with the second agent you linked in that also had Agent_OnLoad. As > an agent developer you will have to select one or the other at build time, > either statically linked in or dynamic. >> You already added the following statement to the JVMTI spec: >> If a /statically linked/ agent L exports a function called Agent_OnLoad_L and >> a function called Agent_OnLoad, the Agent_OnLoad function will be ignored. >> >> Could we say it in a shorter form?: >> If a /statically linked/ agent L exports a function called Agent_OnLoad, >> the Agent_OnLoad function will be ignored. > I believe I copied this from JNI static linking JEP. If so, I'll probably > leave it as is just for consistency with JNI static spec. JVM TI static > linking spec is closely related to JNI static linking spec. >> >> In this context would it be reasonable to add another statement: >> If a /dynamically linked/ agent L exports a function called Agent_OnLoad_L, >> the Agent_OnLoad_L function will be ignored. >>
I'd rather leave this undefined since the behavior is very platform specific. The way we determine if a library is statically linked is by the presence of the Agent_OnLoad_L function. If this function exists in a dynamically linked library, it will be treated as a static library if by some chance it's attempted to be loaded twice, since the symbol will may be visible in the running process. Bob. >> The same questions apply to the Agent_OnAttach and Agent_OnAttach_L entry >> points. >> > I'm out on vacation for a couple of weeks so I'll leave it up to Bob V. and > yourself if you guys want to hash out better/different wording. > > bill >> >> Thanks, >> Serguei >> >> >> On 7/30/13 12:17 PM, bill.pitt...@oracle.com wrote: >>> Thanks Serguei for the comments. Some comments inline. I updated the >>> webrevs at >>> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/ >>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/ >>> >>> http://cr.openjdk.java.net/~bpittore/8014135/javadoc/index.html >>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/jvmti.html >>> >>> >>> On 7/26/2013 5:00 AM, serguei.spit...@oracle.com wrote: >>>> Hi Bill, >>>> >>>> Sorry for the big delay. >>>> >>>> >>>> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.01/ >>>> >>>> >>>> src/share/classes/com/sun/tools/attach/VirtualMachine.java: >>>> >>>> >>>> I'm suggesting to use the reference *<code>Agent_OnAttach[_L]</code>**||* >>>> even more consistently. >>>> (If, in some cases, you prefer the longer form to underline the difference >>>> between >>>> dynamically and statically linked libraries then feel free to leave it as >>>> it is.) >>>> >>>> It would simplify the following fragments: >>>> >>>> 304 * It then causes the target VM to invoke the >>>> <code>Agent_OnAttach</code> function >>>> 305 * or, for a statically linked agent named 'L', the >>>> <code>Agent_OnAttach_L</code> function >>>> ==> >>>> >>>> 304 * It then causes the target VM to invoke the >>>> <code>Agent_OnAttach[_L]</code> function >>>> >>>> 409 * It then causes the target VM to invoke the >>>> <code>Agent_OnAttach</code> >>>> 410 * function or, for a statically linked agent named 'L', the >>>> 411 * <code>Agent_OnAttach_L</code> function as specified in the >>>> ==> >>>> 409 * It then causes the target VM to invoke the >>>> <code>Agent_OnAttach[_L]</code> >>>> 410 * function as specified in the >>>> >>> I left the above as is since it's part of the method description. The >>> following fragments below I simplified as you suggested. >>> >>>> >>>> the following 4 identical fragments: >>>> >>>> 341 * If the <code>Agent_OnAttach</code> function returns >>>> an error >>>> 342 * or, for a statically linked agent named 'L', if the >>>> 343 * <code>Agent_OnAttach_L</code> function returns >>>> 344 * an error. >>>> 375 * If the <code>Agent_OnAttach</code> function returns >>>> an error >>>> 376 * or, for a statically linked agent named 'L', if the >>>> 377 * <code>Agent_OnAttach_L</code> function returns >>>> 378 * an error. >>>> 442 * If the <code>Agent_OnAttach</code> function returns >>>> an error >>>> 443 * or, for a statically linked agent named 'L', if the >>>> 444 * <code>Agent_OnAttach_L</code> function returns an >>>> error >>>> 475 * If the <code>Agent_OnAttach</code> function returns >>>> an error >>>> 476 * or, for a statically linked agent named 'L', if the >>>> 477 * <code>Agent_OnAttach_L</code> function returns >>>> 478 * an error. >>>> ==> >>>> >>>> 336 * If the <code>Agent_OnAttach[_L]</code> function >>>> returns an error. >>>> >>> >>>> >>>> >>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/ >>>> >>>> >>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html >>>> src/share/vm/prims/jvmti.xml >>>> >>>> Lines 442-462: many extra <p/>'s. The fragment does not look well. >>>> I'd suggest to remove most of them. >>>> Also, these lines are too long. Could you make them shorter, please? >>>> The same is applied to other long new lines in this file. >>> Cleaned this up a bit. >>>> >>>> Lines 490-491, 502-503, 505-506: >>>> The same sentence is repeated 3 times: "the agent library may be >>>> statically linked ..." >>>> >>>> 490 Note that the agent library may be statically linked into the >>>> executable >>>> 491 in which case no actual loading takes place. >>> Fixed. >>>> >>>> 501 <code>-agentpath:c:\myLibs\foo.dll=opt1,opt2</code> is specified, the >>>> VM will attempt to >>>> 502 load the shared library <code>c:\myLibs\foo.dll</code>. As >>>> mentioned above, the agent library may be statically linked into the >>>> executable >>>> 503 in which case no actual loading takes place >>>> >>>> 505 Note that the agent library may be statically linked into the >>>> executable >>>> 506 in which case no actual loading takes place. >>>> >>> Tweaked the above a bit to make it less wordy. >>> >>>> Lines 677-678: The dot is missed at the end of line 677: >>>> >>>> 677 and enabled the event >>>> >>> Fixed. >>>> >>>> src/os/posix/vm/os_posix.cpp >>>> >>>> - no comments >>>> >>>> src/os/windows/vm/os_windows.cpp >>>> >>>> - no comments >>>> >>>> src/share/vm/prims/jvmtiExport.cpp >>>> >>>> - no comments >>>> >>>> src/share/vm/runtime/arguments.hpp >>>> >>>> - no comments >>>> >>>> src/share/vm/runtime/os.cpp >>>> >>>> Space is missed after the 'if': >>>> 471 if(entryName != NULL) { >>>> >>> Fixed. >>>> Extra space after the '*': >>>> 483 void * saveHandle; >>>> >>> Fixed. >>>> src/share/vm/runtime/os.hpp >>>> >>>> - no comments >>>> >>>> src/share/vm/runtime/thread.cpp >>>> >>>> The line has been removed: >>>> 3866 break; >>>> >>> Correct, the inner for loop was removed so no need for the break; >>>> >>>> I'm still in process of reading the code. >>>> Another pass is needed to make sure that nothing is missed. >>>> But in general, the code quality is pretty good. >>>> >>>> Thanks, >>>> Serguei >>>> >>>> >>>> >>>> On 7/25/13 10:47 AM, bill.pitt...@oracle.com wrote: >>>>> Still need an official reviewer for the hotspot changes for statically >>>>> linked agents. >>>>> >>>>> thanks, >>>>> bill >>>>> >>>>>> 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 >>>>>> >>>>> >>>> >>> >>> >> >
