RFR: 8003348: SA can not read core file on OS X
Staffan Larsen
staffan.larsen at oracle.com
Mon Mar 18 06:52:56 PDT 2013
Yumin,
I'm ok with these changes now (although I'm not an official Reviewer).
What testing have you done? Do we have existing tests that exercise this code?
/Staffan
On 12 mar 2013, at 22:09, Yumin Qi <yumin.qi at oracle.com> wrote:
> 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/20130318/ac76066c/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list