Yumin,
This looks much better. Thank you for spending the extra time on this. The SA
code in general (and the native code in particular) is quite messy and could
use a lot of cleanup, but we'll have to leave that for later.
A few comments below:
General comment: Please use "#ifdef __APPLE__" consistently (instead of
sometimes using "#ifndef __APPLE__"). I think this makes it easier to read the
code.
MacosxDebuggerLocal.m
getThreadIntegerRegisterSet():
- Should be renamed getThreadIntegerRegisterSetFromCore() or something similar
to indicate that this is used only for core files.
readBytesFromProcess()
- Should also be renamed tom include FromCore.
Java_sun_jvm_hotspot_debugger_bsd_BsdDebuggerLocal_attach0__I()
- It looks like the code for ptrace_attach (lines 528-534) were accidentally
removed
fill_java_threads
- CHECK_EXCEPTION_(0) should be CHECK_EXCEPTION_(false)
- I think cinfos should be of type uint64_t* since it holds some (more or less)
untyped values.
- The inner loop can be made easier to read by breaking out the reading of
cinfos[]. Something like:
for (j = 0; j < len; j += 3) {
lwpid_t lwp = cinfos[j];
uint64_t beg = cinfos[j + 1];
uint64_t end = cinfos[j + 2];
if ((regs.r_rsp < end && regs.r_rsp >= beg) ||
(regs.r_rbp < end && regs.r_rbp >= beg)) {
set_lwp_id(ph, i, lwp);
break;
}
}
- Also note in the above code that rsp and rbp cannot be equal to end, they
have to be strictly smaller.
BsbDebuggerLocal.java
getThreadForIdentifierAddress(Address addr)
- I had intentionally made this throw an exception when fixing JDK-8006423.
This was so that a BsdThread could not be created without setting the
unique_thread_id. Is that no longer valid? Or was this method resurrected by
mistake?
getJavaThreadsInfo
- nit: no need to cast the value from t.getStackBaseValue(), it's already a long
BsdThread.java
BsdThread(BsdDebugger debugger, Address threadIdAddr)
- This constructor was removed in JDK-8006423 - see above.
HotspotAgent.java
connectRemoteDebugger()
- The if-clause is missing a case for "darwin". (Unfortunately remote debugging
does not work even after this fix because of the changes I recently made to
BsdAMD64JavaThreadPDAccess.getThreadProxy(), so that will have to be revisted.)
Thanks,
/Staffan
On 3 mar 2013, at 08:57, Yumin Qi <[email protected]> wrote:
> Hi, all
>
> Please review at the new changes. Include
> 1) use unique_thread_id (which is a 64 bit integer on Macosx) to identify
> thread. Add a function in BsdDebuggerLocal to call the newly added function
> BsdThread.getUniqueThreadId to get this id. Meanwhile, move the code of
> forming threadList in native part to BsdDebuggerLocal.java since the thread
> ids of all java threads can be obtained from Threads and coding is much
> easier and clear.
>
> 2) To have better performance, get all java thread ids, stack infos (stack
> begin, stack end) into one array of long which is decoded in native code and
> used to set thread ids. This save much more time first time to fill java
> thread ids.
>
> 3) remove DarwinVtblAccess.java which added in last version , its
> functionality moved to BsdVtblAccess.java
>
> 4) BugSpotAgent.java no long exists, remove the changes.
>
> 5) remove unsupported platform defs.
>
> http://cr.openjdk.java.net/~minqi/8003348/
>
> Thanks
> Yumin
>
> On 1/22/2013 10:35 PM, Yumin Qi wrote:
>> Hi, Staffan (and Serguei)
>>
>> Made some clean for code.
>> 1) added mach-o file fat header parsing as you suggested.
>> 2) modified get_real_path as you indicated it could run with jre/bin/java
>> 3) moved output information from CommandProcessor.java to PStack.java for
>> printing out pstack not available for Darwin.
>> 4) code clean, file header update.
>>
>> Please take a look at same location.
>>
>> Thanks
>> Yumin
>>
>> On 1/18/2013 3:58 AM, Staffan Larsen wrote:
>>> Thanks for doing this Yumin!
>>>
>>> I tried to apply you patch and run it, but I can't get SA to open a core
>>> file. You can see the exception I get below. Is there some kind of setup I
>>> need to do? This is against a jvmg build of Hotspot.
>>>
>>> I also noticed that you forgot to update BugSpotAgent.java with the same
>>> changes as in HotspotAgent.java. This makes the jstack tool fail.
>>>
>>> I will look at the changes more closely.
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>
>>>
>>> Opening core file, please wait...
>>> Unable to open core file
>>> /cores/core.79028:
>>>
>>> Doesn't appear to be a HotSpot VM (could not find symbol "gHotSpotVMTypes"
>>> in
>>> remote process)
>>> sun.jvm.hotspot.debugger.DebuggerException: Doesn't appear to be a HotSpot
>>> VM (could not find symbol "gHotSpotVMTypes" in remote process)
>>> at sun.jvm.hotspot.HotSpotAgent.setupVM(HotSpotAgent.java:385)
>>> at sun.jvm.hotspot.HotSpotAgent.go(HotSpotAgent.java:287)
>>> at sun.jvm.hotspot.HotSpotAgent.attach(HotSpotAgent.java:146)
>>> at sun.jvm.hotspot.CLHSDB.attachDebugger(CLHSDB.java:188)
>>> at sun.jvm.hotspot.CLHSDB.run(CLHSDB.java:55)
>>> at sun.jvm.hotspot.CLHSDB.main(CLHSDB.java:35)
>>> hsdb> Input stream closed.
>>>
>>>
>>> On 17 jan 2013, at 22:23, Yumin Qi<[email protected]> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please review for the changes for SA to read core file on Mac OS X,
>>>> this is feature is not implemented on previous releases.
>>>> This change made for SA to work on core file on Darwin, but still some
>>>> function not fixed, such as 'pstack'. This is intended to integrate into 8.
>>>>
>>>> http://cr.openjdk.java.net/~minqi/8003348/
>>>>
>>>> Please take some time since the code change is a little bigger.
>>>>
>>>> Thanks very much
>>>> Yumin
>
