RFR: 8010278 SA: provide mechanism for using an alternative SA debugger back-end.

Staffan Larsen staffan.larsen at oracle.com
Tue Jun 25 10:22:37 PDT 2013 

Looks good me (still).

/Staffan

On 25 jun 2013, at 12:13, Kevin Walls <kevin.walls at oracle.com> 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.larsen at oracle.com>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> On 19 jun 2013, at 14:40, Kevin Walls<kevin.walls at 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.walls at 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-)
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130625/966c5e76/attachment-0001.html

More information about the serviceability-dev mailing list