RFR: 8003348: SA can not read core file on OS X
Yumin Qi
yumin.qi at oracle.com
Wed Mar 6 08:54:15 PST 2013
Thanks, Serguei
Will take care what you have indicated but refactoring code for
ps_core.c (Pgrab_core). Maybe in future, code cleaning should include
this, the better choice I think is regroup code to detail in specific
platform, such as with new file ps_core_darwin.c etc but not now.
/Yumin
On 3/5/2013 8:36 PM, serguei.spitsyn at oracle.com wrote:
>
> 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/20130306/1b08789e/attachment-0001.html
More information about the serviceability-dev
mailing list