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

Tue Jun 25 03:13:48 PDT 2013

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/75cda711/attachment-0001.html 