Thanks Dmitry and Staffan!
Kevin
On 25/06/13 19:47, Dmitry Samersoff wrote:
Kevin,
Looks good for me!
Thumbs up.
-Dmitry
On 2013-06-25 14:13, Kevin Walls wrote:
OK I'm coming back with a bit more change as well as those points.
http://cr.openjdk.java.net/~kevinw/8010278/webrev.03/
To clarify what's going on: there are actually two sides to this which I
had probably mixed up more than was necessary:
*existing debugger:* the HotSpotAgent is being created by some
application, with an existing JVMDebugger object which is already
attached to some target. This does not actually require the setting of
a property to distinguish it from existing usage: the new
HotSpotAgent(JVMDebugger) constructor is used. This is only the case
when a 3rd party debugger tool creates its own JVMDebugger
implementation and instance, to share among different SA Tools (within
the same debugger process).
*alternate debugger:* the HotSpotAgent is being created in the
traditional way by some existing SA Tool, but we want to respect a
property which tells it to use some other JVMDebugger implementation.
In HotSpotAgent, we can distinguish these in setupDebugger, based on
whether HotSpotAgent has a JVMDebugger object (existing debugger), or
has the sa.altDebugger set. There was no need for the new value for the
startupMode, no need for the doAttach flag. This works
These two cases are now handled by distinct method calls from
setupDebugger, which makes it much clearer I think. And I've tested
both cases, it looks good, and the existing behaviour remains unchanged.
In the "alternate" case, you can actually use all the existing
command-line tools like jstack, by using -J- to set bootclasspath to
include your implementation/sa jars, and -J-Dsa.altDebugger to set the
property.
Thanks for the comments so far - this version has everything I think...
(only problem I know I have is it has been a while so the patch doesn't
apply to latest hotspot, will need to refresh...).
Thanks
Kevin
On 24/06/13 16:39, Dmitry Samersoff wrote:
Thanks Kevin,
You answered all my questions ;)
-Dmitry
On 2013-06-24 18:53, Kevin Walls wrote:
Thanks Dmitry --
I'll come back with an update, but just a couple of responses...
On 24/06/13 15:01, Dmitry Samersoff wrote:
On 2013-06-24 17:48, Kevin Walls wrote:
Thanks Dmitry --
CommandProcessor.java:
Yes, should make quit volatile.
HSDB.java:
The idea is to avoid System.exit as it is really unfriendly when another
app invokes the tool. The existing usage pattern is:
new HSDB(args).run();
..so avoiding System.exit() means making run() guard against being
called when the args were bad, i.e. the usage error was issued. I could
change main() here to check something and not call run, but the general
pattern of the tools is to instantiate them and call run(), so I thought
it best that run should have this check?
I don't fill myself comfortable with create a thread and just exit
because of a previous error. So IMHO, it's better to don't call run at
all.
I don't think we're creating a new thread for HSDB?
(although Tool implements Runnable, HSDB doesn't extend Tool like other
classes do, if that's where this comes from?)
I think run() needs to protect itself from creating a GUI when the app
should not be up, due to an argument error. The flag could have been
called "initialized" but that is misleading: there's a lot more
"initialisation" yet to come when you get to run()!
Let me know if you have more thoughts on what makes you uncomfortable
here. 8-)
HotSpotAgent.java:
1
Yes maybe we don't need the doAttach flag. I'll try that out.
OK. Thank you!
2
Do you mean to not have the call to setupDebuggerAlternate() alongside
the alternative calls to setupDebuggerSolaris/Linux/Win32/etc...?
Maybe it doesn't have the same dependencies as those methods, but it
does do the kinds of things:
call setupJVMLibNames, and also set the MachineDescription. I think it
is still a parallel method to those for the other platforms, so this
seems like the appropriate point to call it? If we move the call to
somewhere earlier, we would still have to check the "sa.altDebugger"
property in setupDebugger(), to make sure we DON'T call one of the other
setupDebuggerXX methods.
(If I didn't properly understand what you meant, let me know!)
Now if we try to run SA on non-supported platform CPU combination it
fails ever if we provide our own backend for this platform.
I'm not sure - is it OK or not.
Yes, I see what you mean, the UnsupportedPlatformException - we are
still tied to the running JVM's PlatformInfo implementation. I should
be able to reorder this a little to solve that...
Thanks
Kevin
3
Oh you found my "XXX" comment, yes... 8-)
That needs tidying up.
VM.java:
On the properties, we are looking up a build number so we can issue a
warning about a mismatch.
I had also noticed we corrupt "derivedPointer" to "derivedPrinter" in a
few places, which I'll correct while doing this. 8-)
Yes disableDerivedPrinterTableCheck (which we should call
disableDerivedPointerTableCheck in future) is set even if you can't read
the "sa.properties" file, but it's set from System.getProperties, i.e.
not related, just happens to be another part of the static {} block.
That seems OK?
OK.
LinuxAddress.java, LinuxOopHandle.java :
Added public so they are accessible to other code, e.g. so an external
unrelated tool can create a LinuxAddress.
It's better to add a comment that these classes is intended to be used
by external tools.
ClassDump.java:
The change here is about letting it get initialized somewhere other than
main(). i.e if the Tool is invoked without main, and originally main is
where it initializes its class filter and output directory. (It doesn't
use the args from main for these) So it's moved...
Could you add a comment explaining it?
Let me rebuild and test with these changes and come back with an updated
webrev.
Thanks!
-Dmitry
Thanks
Kevin
On 24/06/13 13:28, Dmitry Samersoff wrote:
Kevin,
* CommandProcessor.java:
"quit" should be volatile.
* HSDB.java:
argError logic is not clean to me. Is it possible to just do return
after doUsage or use System.exit()?
* HotSpotAgent.java:
1.
You can probably check whether "debugger" variable is already set or
not
and get rid of "doAttach" and thus simplify code a bit.
2.
setupDebuggerAlternate doesn't depend to
value of os and cpu, so it might have sense to move this call to upper
level.
672:
Comment is redundant.
* VM.java:
After your changes some code in static initializer
e.g. disableDerivedPrinterTableCheck
is executed ever in case of error.
I'm not sure we should try to recover here - if we can't load
properties, something very bad is happening.
LinuxAddress.java, LinuxOopHandle.java :
What is the reason to make it public?
* ClassDump.java:
Does these changes related to this CR?
On 2013-06-24 15:24, Kevin Walls wrote:
Thanks Staffan,
Yes, the GUI does still close and the standalone HSDB ends, without
forcibly terminating the process if it's initiated by some other app.
Can I get anybody else interested in reviewing, commenting, etc...?
Thanks
Kevin
On 24/06/13 12:18, Staffan Larsen wrote:
On 24 jun 2013, at 11:20, Staffan Larsen<staffan.lar...@oracle.com>
wrote:
On 19 jun 2013, at 14:40, Kevin Walls<kevin.wa...@oracle.com>
wrote:
Hi Staffan --
And apologies from me for the slow turnaround. 8-)
Thanks for the suggestions, implementing them all.
Thanks.
http://cr.openjdk.java.net/~kevinw/8010278/webrev.02/
Also adding the same kind of change to the HSDB tool, a few changes
there to get the gui closing without using System.exit.
So how do I now exit the HSDB tool? Closing the window won't exit.
File->Exit won't exit. Or did I miss something?
I did miss that the workerThread is also terminated in closeUI() and
that causes the app to exit.
This looks good. Reviewed.
/Staffan
/Staffan
Additional feedback gratefully received...
Thanks
Kevin
On 05/06/13 12:00, Staffan Larsen wrote:
Some comments (sorry it took so long):
CLHSDB.java
- Can you move the jvmDebugger field to where the other fields are
in the class? Make it private, too.
- Remove start() and make run() public?
- Perhaps improve on the comment in run() saying that either
jvmDebugger OR pidText OR {execPath, coreFileName} should be set.
HotspotAgent.java
- Should sa.altHotSpotAgent be called sa.altDebugger ?
Tool.java
- Make jvmDebugger private.
Regards,
/Staffan
On 23 maj 2013, at 15:23, Kevin Walls<kevin.wa...@oracle.com>
wrote:
Forgot to mention:
I moved the ClassDump Tool around a little so it was usable via a
route other than calling main(), and added the constructor that
takes a String for the pkgList, which saves using the system
property to communicate what classes you want to dump
(sun.jvm.hotspot.tools.jcore.PackageNameFilter has that
constructor already).
Actually, considering the package filter name is always going to
be sun.jvm.hotspot.tools.jcore.PackageNameFilter, let's have that
as a default value when we call getProperty. Similarly the
getProperty for outputDir, and at that point I stop tweaking.
A little indenting was off also and I added a comment, so I redid
the webrev:
http://cr.openjdk.java.net/~kevinw/8010278/webrev.01/
Thanks
Kevin
On 23/05/13 11:00, Kevin Walls wrote:
Hi,
As the Serviceability Agent has been _the_ new and interesting
way to find things post-mortem in the JVM [1], I'd like to
propose an update which continues that tradition.
8010278 SA: provide mechanism for using an alternative SA
debugger back-end.
https://jbs.oracle.com/bugs/browse/JDK-8010278
http://cr.openjdk.java.net/~kevinw/8010278/webrev.00/
This is about making the SA more flexible, so we aren't tied to
the given native libraries to open cores/memory dumps. Given
this change, a 3rd party debugger or tool can interact freely
with the SA tools (StackTrace, ObjectHistogram, etc...) and
provide its own backend implementation to actually open a
core/memory dump.
Primarily for platform-independent core file debugging. If you
ever had to open a "foreign" core, find the right hardware,
etc... this is relevant. I'm thinking of
https://java.net/projects/kjdb which can serve as a proof of
concept.
The changes are:
The main redirection is in HotSpotAgent.java, where we respect a
property (i.e. -Dsa.altHotSpotAgent=...) to name an alternate
debugger.
Remove calls to System.exit.
Tool classes (and CLHSDB) should have a constructor that takes a
JVMDebugger, to remove the assumption that a Tool's JVM will
only
ever contain one debugee. It doesn't address that VM is a
singleton and if a tool opens multiple sessions then they would
need to be from the same JVM version.
Thanks
Kevin
[1] If you weren't in a circa 1.4.2 demo of the SA when all you
had previously was a few fragile dbx macros, that got you a few
very specific details, the night vs. day comparison of no SA vs.
SA could be missed. 8-)
