RFR: 8003348: SA can not read core file on OS X
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Mar 7 16:01:52 PST 2013
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/hotspot-runtime-dev/attachments/20130307/77491449/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list