RFR: 8003348: SA can not read core file on OS X
Yumin Qi
yumin.qi at oracle.com
Thu Mar 7 15:55:19 PST 2013
Hi, Serguei
Thanks for the review, I reviewed the part of Pgrab_core, you are
right, I now put the code into two chunks: APPLE and none APPLE.
Will send another webrev for you tonight --- add all your concerns.
See answers below:
On 3/7/2013 3:32 PM, serguei.spitsyn at oracle.com wrote:
> 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");
Yes, will.
>
>
> 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) {
Will try to make it nice.
> 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
>
OK, will change.
> 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;
Will change.
> 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) {
>
Will change.
> ||*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.
>
Will change to 4.
The SA code has no consistent coding style, some place you will see 3,
some place 2 or 4.
For Java code, I will change them to have 4 spaces. For C, have done
some with 2, but still large amount of code ended with 3.
This needs future clean work.
>
> 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 }
Oh, I see, the comments reversed.
>
> *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 }
>
Yes.
> 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.
>
My work is making SA reading core on Macosx work, now we have a working
version! thanks.
Yumin
>
> 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/9953b5d8/attachment-0001.html
More information about the serviceability-dev
mailing list