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");
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) {
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
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;
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) {
||*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.
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 }
*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 }
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.
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
