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

Tue Jun 25 13:24:32 PDT 2013

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