RFR: 8003348: SA can not read core file on OS X
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Mar 7 15:32:50 PST 2013
Hi Yumin,
No insisting on refactoring, it is just desirable. :)
*
agent/src/os/bsd/symtab.c*
Need a cleanup:
54 // #define NAMELEN 4096
78 // dysymtab_command dysymtabcmd;
114 // guarantee(symtab->hash_table, "unexpected failure: dbopen");
This function is too big, it would be nice to factor out some
of its body fragments as functions:
55 struct symtab* build_symtab(int fd) {
The line 137 is too long. You can do like this:
int size = symtabcmd.strsize * sizeof(char);
if (read(fd, (void *)(symtab->strs), size) != size) {
Space is missed:
145 //fix size
No point to start new line if similar fragments like that do not have it:
153 symtab->symbols[i].size =
154 symtabcmd.strsize - symtab->symbols[i].size;
Empty line is needed after the structure definition:
199 void *c_data;
200 };
201 // read symbol table from given fd.
202 struct symtab* build_symtab(int fd) {
||*agent/src/share/classes/sun/jvm/hotspot/BsdVtblAccess.java*
Need a consistent indentation (4?) as they are 2 and 3.
For instance, the BsdDebuggerLocal.java has indent == 4.
The flag name " newVT" does not match the comments, should it be named
"oldVT" ? :
46 if (newVT) {
47 // old C++ ABI
48 vt = isDarwin ? "_vt_" : "__vt_";
49 } else {
50 // new C++ ABI
51 vt = "_ZTV";
52 }
*agent/src/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java*
Incorrect indent, it must be 4:
86 public long getUniqueThreadId() {
87 return unique_thread_id;
88 }
Sorry for paying too much attention to indentation!
It is because the SA indentation is a real mess. :)
But this project is a nice progress in the SA area!
In fact, it is a lot of work.
I bet, you spent a lot of time debugging this code.
Thanks,
Serguei
On 3/6/13 8:54 AM, Yumin Qi wrote:
> 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/20130307/47e28585/attachment.html
More information about the serviceability-dev
mailing list