RFR: 8003348: SA can not read core file on OS X
Yumin Qi
yumin.qi at oracle.com
Thu Mar 7 22:22:35 PST 2013
Serguei and Saffan,
Please take look at the same link for new webrev.
Thanks
Yumin
On 3/7/2013 4:01 PM, serguei.spitsyn at oracle.com wrote:
> Thank you for making the suggested changes!
> Serguei
>
> On 3/7/13 3:55 PM, Yumin Qi wrote:
>> 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/ae33003c/attachment-0001.html
More information about the serviceability-dev
mailing list