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.
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
