First round of comments. Overall MACH support and extended DWARF support looks very good, thanks for implementing it!
But I am not sure about gdb-integration-* and gdb-v8-support.py in samples/tools
directories. I do not think we should actually put it there. Can we exclude these files from the CL? We can try to find a more appropriate place for them later. http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc File src/gdb-jit.cc (right): http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode47 src/gdb-jit.cc:47: #endif I don't think we are using macros for anything like this in the code base. Can we use typedef here instead? typedef ELF* DebugObject; or something similar. http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode59 src/gdb-jit.cc:59: mach_o_(0), Instead of duplicating code and members replace elf with a member of type DebugObject (OBJ_FORMAT in your code). http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode263 src/gdb-jit.cc:263: virtual void WriteBody(Writer::Slot<Header> header, Writer* writer) { Seems to be shared between ELFSection and MachOSection. Extract to a base class for both sections? http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode273 src/gdb-jit.cc:273: virtual bool WriteBody(Writer* writer) { Ditto. http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode547 src/gdb-jit.cc:547: ASSERT(w->position() == 0); Can you split this method into several like it is done for ELF? http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1019 src/gdb-jit.cc:1019: : MachOSection("__debug_info", "__DWARF", 1, one argument per line. http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1129 src/gdb-jit.cc:1129: for (int context_slot = 0; context_slot < context_slots; for(x; y; z) http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1189 src/gdb-jit.cc:1189: : MachOSection("__debug_abbrev", "__DWARF", 1, Argument per line http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1832 src/gdb-jit.cc:1832: void __gdb_print_v8_object(MaybeObject* object) { Frankly speaking I don't see any reason to add this instead of just calling Print directly from gdb prompt http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1861 src/gdb-jit.cc:1861: bool dump_if_enabled, something with indentation. http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1871 src/gdb-jit.cc:1871: OS::SNPrintF(Vector<char>(file_name, kMaxFileNameSize), "%s%s%d%s", One argument per line. (somehow I missed this place in the previous commits, sorry). http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.h File src/gdb-jit.h (right): http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.h#newcode118 src/gdb-jit.h:118: Script* script = NULL, C++ Style Guide does not allow default arguments. I am sorry I've missed this in case of script. Can you change all callers to pass NULL explicitly? http://codereview.chromium.org/6995161/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
