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

Kevin Walls kevin.walls at oracle.com
Mon Jun 24 07:53:06 PDT 2013 

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

More information about the serviceability-dev mailing list