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
