On Aug 6, 2013, at 3:10 PM, serguei.spit...@oracle.com wrote: > On 8/6/13 11:57 AM, serguei.spit...@oracle.com wrote: >> On 8/5/13 7:41 AM, Bob Vandette wrote: >>> 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. >> (I've added Alan and Coleen to the cc-list) >> >> It is still not clear to me how the second load might happen. > > It is inaccurate statement above. > I wanted to say "such a second load" that will determine the dynamically > loaded agent as statically loaded.
Could you please restate the issue you'd like addressed. I don't think we can guarantee that an Agent_OnLoad_L symbol located in a shared library will not be visible to our running process on all operating system platforms. Bob. > > Thanks, > Serguei > >> >> The agent is determined as statically loaded if the Agent_OnLoad_L function >> is found in the main program, not in the dynamic library. >> At least, the dll_lookup is done for the main program (for >> DefaultProcessHandle). >> If the agent is loaded dynamically, the Agent_OnLoad_L function in the agent >> library >> can be ignored because the dll_lookup in the main program will not find it. >> In a case of the second load this function will be ignored again. >> So that there is no conflict with the dynamically loaded agent here. >> >> We can skip this issue for now. >> Alan is going to review the JEP from the dynamic Attach point of view and >> hopefully will pay attention to these details. >> >> Thanks, >> Serguei >> >>> >>> 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 >>>>>>>>> >>>>>> >> >
