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...@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.spit...@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.spit...@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.spit...@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.spit...@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...@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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
