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

Dmitry Samersoff dmitry.samersoff at oracle.com
Mon Jun 24 08:39:01 PDT 2013 

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

More information about the serviceability-dev mailing list