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.lar...@oracle.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> On 19 jun 2013, at 14:40, Kevin Walls<kevin.wa...@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.wa...@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.
