RFR: 8003348: SA can not read core file on OS X
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Mar 5 20:36:43 PST 2013
Hi Yumin,
A partial review below.
*
agent/src/os/bsd/MacosxDebuggerLocal.m
*
152 listAdd_ID = (*env)->GetMethodID(env, listClass, "add", "(Ljava/lang/Object;)Z");
153 CHECK_EXCEPTION;
154 getJavaThreadsInfo_ID = (*env)->GetMethodID(env, cls, "getJavaThreadsInfo",
155 "()[J");
156
157 init_libproc(getenv("LIBSAPROC_DEBUG") != NULL);
Is CHECK_EXCEPTION needed after the line #154?
The indent is 3 instead of 2:
Lines 360-410, 775-788
||*agent/src/os/bsd/libproc.h*
36 #ifdef __APPLE__
. . .
46 #ifndef bool
47 typedef int bool;
48 #define true 1
49 #define false 0
50 #endif // bool
. . .
57 #else // __APPLE__
. . .
76 // This C bool type must be int for compatibility with BSD calls and
77 // it would be a mistake to equivalence it to C++ bool on many platforms
78 typedef int bool;
79 #define true 1
80 #define false 0
81
82 #endif // __APPLE__
The bool, true and false are defined the same way for APPLE and not APPLE.
Would it make sense to define it just once?
||*agent/src/os/bsd/ps_core.c*
Need a clean up:
869 // thread_command thrcmd;
Space is missed after "if", wrong indent at line #873:
872 if(read(fd, (void *)&fhead, sizeof(mach_header_64)) != sizeof(mach_header_64)) {
873 goto err;
Lines 893, 1087 are too long.
The function read_core_segments() is big and not well readable.
Would it make sense to consider to separate some of its internals as a
local functions.
For instance, the lines 921-950 are good candidates for it.
The indent is 3:
1015 if (exists(filepath)) {
1016 strcpy(rpath, filepath);
1017 return true;
1018 }
I did not understand the comments, probably, some words are missing:
1070 mach_header_64 header; // used to check if a file header in segment
. . .
1110 // this is the file begining to core file.
Not consistent, other fragments before used "goto err;" :
1122 return false; // error
. . .
1133 return false;
Too many ifdef'ed fragments with __APPLE__
Is it possibler to combine them into bigger chunks or refactor in a
different way?
For instance, the function Pgrab_core() is not readable.
It'd be better to have two separated versions of the function for apple
and not apple.
Still to review:
||*agent/src/os/bsd/symtab.c*
||*agent/src/os/bsd/symtab.h*
||*agent/src/share/classes/sun/jvm/hotspot/BsdVtblAccess.java*
||*agent/src/share/classes/sun/jvm/hotspot/HotSpotAgent.java*
||*agent/src/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java*
*agent/src/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java*
||*agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java*
||*agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java*
||*agent/src/share/classes/sun/jvm/hotspot/tools/PStack.java*
||*agent/src/share/classes/sun/jvm/hotspot/utilities/PlatformInfo.java*
||*agent/src/share/classes/sun/jvm/hotspot/CommandProcessor.java*
||*agent/src/share/native/sadis.c*
||*make/bsd/makefiles/saproc.make*
Thanks,
Serguei
On 3/2/13 11:57 PM, Yumin Qi 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.qi at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130305/a934d0e0/attachment-0001.html
More information about the serviceability-dev
mailing list