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

Kevin Walls kevin.walls at oracle.com
Mon Jun 24 06:48:55 PDT 2013 

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?


HotSpotAgent.java:

1
Yes maybe we don't need the doAttach flag.  I'll try that out.

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


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?


LinuxAddress.java, LinuxOopHandle.java :
Added public so they are accessible to other code, e.g. so an external 
unrelated tool can create a LinuxAddress.


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


Let me rebuild and test with these changes and come back with an updated 
webrev.

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