RFR: 8003348: SA can not read core file on OS X
Yumin Qi
yumin.qi at oracle.com
Tue Mar 12 14:09:40 PDT 2013
Serguei,
Thanks.
Staffan, can you have a look at the new changes, concern?
Thanks
Yumin
On 3/12/2013 12:52 PM, serguei.spitsyn at oracle.com wrote:
> Yumin,
>
> The updated webrev looks good.
> Just some simple cosmetic and debug comments below.
>
>
> *agent/src/os/bsd/MacosxDebuggerLocal.m*
>
> Nice refactoring!
>
> 350 /** Only used for core file reading, set thread_id for threads which got after core file parsing.
> 351 When parsing core file on Mac OS X, thread context is available but thread id.
> 352 We identify them as java threads by checking if a thread's rsp or rbp within a java thread's
> 353 stack(stack info is recoreded when it is created). Note Macosx uses unique_thread_id which is
> 354 different from other platforms though printed ids are still pthread id.
> 355 Function BsdDebuggerLocal.getJavaThreadsInfo returns an array of long integers to host
> 356 all java threads' id, stack_start, stack_end as:
> 357 [uid0, stack_start0, stack_end0, uid1, stack_start1, stack_end1, ...]
> 358
> 359 The work cannot be done at init0 since Threads is not available yet(VM not initialized yet).
> 360 This function should be called only once if succeeded */
> It'd be better to start lines 351-360 with the '*'.
> The sentence at line 351 is not clear.
>
>
> 393 /* core file only, called from Java_sun_jvm_hotspot_debugger_bsd_BsdDebuggerLocal_getThreadIntegerRegisterSet0 */
> 394 jlongArray getThreadIntegerRegisterSetFromCore(JNIEnv *env, jobject this_obj, long lwp_id) {
> 395 // On Mac OS X, we can not get thread_id from x86Thread_State_t, but they are recorded in JavaThread of JVM
> The comment lines 393 and 395 are two long and can be split in two.
>
> 440 regs[REG_INDEX(RDI)] = gregs.r_rdi;
> 441 regs[REG_INDEX(RIP)] = gregs.r_rip;
> 442 regs[REG_INDEX(CS)] = gregs.r_cs;
> 443 regs[REG_INDEX(RSP)] = gregs.r_rsp;
> 444 regs[REG_INDEX(SS)] = gregs.r_ss;
> 445 regs[REG_INDEX(FSBASE)] = 0;
> 446 regs[REG_INDEX(GSBASE)] = 0;
> 447 regs[REG_INDEX(DS)] = gregs.r_ds;
> 448 regs[REG_INDEX(ES)] = gregs.r_es;
> 449 regs[REG_INDEX(FS)] = gregs.r_fs;
> 450 regs[REG_INDEX(GS)] = gregs.r_gs;
> 451 regs[REG_INDEX(TRAPNO)] = gregs.r_trapno;
> 452 regs[REG_INDEX(RFL)] = gregs.r_rflags;
>
> At least, the lines 442, 444, 447-450 and 452 can be aligned as 440-441.
>
> 652 print_error("attach: Failed to correctly attach to VM. VM might HANG! [PTRACE_CONT failed, stopped by %d]\n", WSTOPSIG(status));
> The line above is too long. You can split it like this:
> 652 print_error("attach: Failed to correctly attach to VM. VM might HANG! "
> "[PTRACE_CONT failed, stopped by %d]\n", WSTOPSIG(status));
>
> *agent/src/os/bsd/Makefile*
>
> The lines 25 and 55 are too long.
>
> |
> |*agent/src/os/bsd/libproc_impl.c*
>
> | I see you've fixed many indents in this file.
> |
>
>
> || *agent/src/os/bsd/ps_core.c*
>
> 565 print_debug(" r_r15: 0x%" PRIx64 "\n", threadinfo->regs.r_r15);
> 566 print_debug(" r_r14: 0x%" PRIx64 "\n", threadinfo->regs.r_r14);
> 567 print_debug(" r_r13: 0x%" PRIx64 "\n", threadinfo->regs.r_r13);
> 568 print_debug(" r_r12: 0x%" PRIx64 "\n", threadinfo->regs.r_r12);
> 569 print_debug(" r_r11: 0x%" PRIx64 "\n", threadinfo->regs.r_r11);
> 570 print_debug(" r_r10: 0x%" PRIx64 "\n", threadinfo->regs.r_r10);
> 571 print_debug(" r_r9: 0x%" PRIx64 "\n", threadinfo->regs.r_r9);
> 572 print_debug(" r_r8: 0x%" PRIx64 "\n", threadinfo->regs.r_r8);
> 573 print_debug(" r_rdi: 0x%" PRIx64 "\n", threadinfo->regs.r_rdi);
> 574 print_debug(" r_rsi: 0x%" PRIx64 "\n", threadinfo->regs.r_rsi);
> 575 print_debug(" r_rbp: 0x%" PRIx64 "\n", threadinfo->regs.r_rbp);
> 576 print_debug(" r_rbx: 0x%" PRIx64 "\n", threadinfo->regs.r_rbx);
> 577 print_debug(" r_rdx: 0x%" PRIx64 "\n", threadinfo->regs.r_rdx);
> 578 print_debug(" r_rcx: 0x%" PRIx64 "\n", threadinfo->regs.r_rcx);
> 579 print_debug(" r_rax: 0x%" PRIx64 "\n", threadinfo->regs.r_rax);
> 580 print_debug(" r_fs: 0x%" PRIx32 "\n", threadinfo->regs.r_fs);
> 581 print_debug(" r_gs: 0x%" PRIx32 "\n", threadinfo->regs.r_gs);
> 582 print_debug(" r_rip 0x%" PRIx64 "\n", threadinfo->regs.r_rip);
> 583 print_debug(" r_cs: 0x%" PRIx64 "\n", threadinfo->regs.r_cs);
> 584 print_debug(" r_rflags: 0x%" PRIx64 "\n", threadinfo->regs.r_rflags);
> 585 print_debug(" r_rsp: 0x%" PRIx64 "\n", threadinfo->regs.r_rsp);
>
>
> The lines can be aligned better and printed info will be more readable too:
> 571-572,580-583
>
>
> 622 print_debug("segment added: %" PRIu64 " 0x%" PRIx64 " %d\n", segcmd.fileoff, segcmd.vmaddr, segcmd.vmsize);
> 816 print_debug("map_info %d: vmaddr = 0x%016" PRIx64 " fileoff = %" PRIu64 " vmsize = %" PRIu64 "\n", j, iter->vaddr, iter->offset, iter->memsz);
> The print_debug lines are too long.
>
>
> One empty line extra: 272-273.
>
>
> 956 if (read_core_segments(ph) != true)
> 957 goto err;
> 958
> 959 // allocate and sort maps into map_array, we need to do this
> 960 // here because read_shared_lib_info needs to read from debuggee
> 961 // address space
> 962 if (sort_map_array(ph) != true)
> 963 goto err;
> 964
> 965 if (read_shared_lib_info(ph) != true)
> 966 goto err;
> 967
> 968 // sort again because we have added more mappings from shared objects
> 969 if (sort_map_array(ph) != true)
> 970 goto err;
> 971
> 972 if (init_classsharing_workaround(ph) != true)
> 973 goto err;
>
> Probably have to print a debug error message for before "goto err;".
>
>
> Thanks,
> Serguei
>
> On 3/7/13 10:22 PM, Yumin Qi wrote:
>> 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/hotspot-runtime-dev/attachments/20130312/cc536c2c/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list