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 <yumin...@oracle.com> 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<yumin...@oracle.com> 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
> 

Reply via email to