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

Dmitry Samersoff dmitry.samersoff at oracle.com
Mon Jun 24 07:01:50 PDT 2013 

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.

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

> 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