Extend gdb-jit support (OSX/locals+parameters/prettyprint)

Extend gdb-jit support (OSX/locals+parameters/prettyprint)

Generate Mach-O in-memory objects for OSX. Dump locals and parameters
for non-optimized frames.

Unfortunately, it seems like more-recent-GDB on OSX there is a little
temperamental (eg, the version from macports will be missing symbols
from gdb-integration_g when the version included in xcode will not--
and this is with --gdbjit off).

Includes some Python scripts to make dealing with V8 values in gdb more
pleasant. 

Patch by Luke Zarko.

Committed: http://code.google.com/p/v8/source/detail?r=8483

[zarko, 2011-06-13 20:48:17]

If you get a chance, please take a look at this changelist.

Although it's not essential-- do you have any pointers on where to start looking
to understand the format for deoptimization data?

Thanks!

[Vyacheslav Egorov (Chromium), 2011-06-23 11:14:47]

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?

[Vyacheslav Egorov (Chromium), 2011-06-23 11:28:37]

A bit more comments.

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#newcode1293
src/gdb-jit.cc:1293: int current_abbreviation = 4;
Move it to the beginning of the function and use instead of 1, 2, 3

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1318
src/gdb-jit.cc:1318: for (int context_slot = 0; context_slot < context_slots;
for(x;
    y;
    z)

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1320
src/gdb-jit.cc:1320: w->WriteULEB128(current_abbreviation++);
Seems to be duplicated. Extract to a method?

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1329
src/gdb-jit.cc:1329: for (int internal_slot = 0; internal_slot < internal_slots;
for(x;
    y;
    z)

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1331
src/gdb-jit.cc:1331: w->WriteULEB128(current_abbreviation++);
Seems to be duplicated. Extract to a method?

Maybe extract the whole loop into method.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1341
src/gdb-jit.cc:1341: w->WriteULEB128(current_abbreviation++);
Partially duplicated. If possible try to unify with method extracted for the
previous loops.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1368
src/gdb-jit.cc:1368: w->WriteULEB128(current_abbreviation++);
Duplicated?

[zarko, 2011-06-27 22:24:29]

gdb on OSX is really touchy!

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
Done (though I have kept the __MACH_O/__ELF defines for purposes of an
initializer down below; if this is still not a great idea I can try to change it
further).

On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> 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),
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Instead of duplicating code and members replace elf with a member of type
> DebugObject (OBJ_FORMAT in your code).

Done.

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) {
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Seems to be shared between ELFSection and MachOSection. Extract to a base
class
> for both sections?

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode273
src/gdb-jit.cc:273: virtual bool WriteBody(Writer* writer) {
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Ditto.

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode547
src/gdb-jit.cc:547: ASSERT(w->position() == 0);
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Can you split this method into several like it is done for ELF?

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1019
src/gdb-jit.cc:1019: : MachOSection("__debug_info", "__DWARF", 1,
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> one argument per line.

Done.

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;
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> for(x;
>     y;
>     z)

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1189
src/gdb-jit.cc:1189: : MachOSection("__debug_abbrev", "__DWARF", 1,
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Argument per line

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1293
src/gdb-jit.cc:1293: int current_abbreviation = 4;
On 2011/06/23 11:28:37, Vyacheslav Egorov wrote:
> Move it to the beginning of the function and use instead of 1, 2, 3

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1318
src/gdb-jit.cc:1318: for (int context_slot = 0; context_slot < context_slots;
On 2011/06/23 11:28:37, Vyacheslav Egorov wrote:
> for(x;
>     y;
>     z)

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1329
src/gdb-jit.cc:1329: for (int internal_slot = 0; internal_slot < internal_slots;
On 2011/06/23 11:28:37, Vyacheslav Egorov wrote:
> for(x;
>     y;
>     z)

Done.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1832
src/gdb-jit.cc:1832: void __gdb_print_v8_object(MaybeObject* object) {
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> Frankly speaking I don't see any reason to add this instead of just calling
> Print directly from gdb prompt

gdb was having some trouble finding the symbol for Print; exposing it as a plain
cdecl makes it easier.

http://codereview.chromium.org/6995161/diff/1/src/gdb-jit.cc#newcode1861
src/gdb-jit.cc:1861: bool dump_if_enabled,
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> something with indentation.

Done.

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",
On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> One argument per line.
> 
> (somehow I missed this place in the previous commits, sorry).

Done.

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,
No problem; I think I got the other callers.

On 2011/06/23 11:14:47, Vyacheslav Egorov wrote:
> 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?

[Vyacheslav Egorov (Chromium), 2011-06-28 10:33:31]

I think it's ready to land but I really want to exclude contents of samples
directory for now. I strongly feel it does not belong there. Is it fine with you
Luke?

If it is fine please submit another CL with those files excluded and I'll land
it. (tools/gdb-* is fine though).

[zarko, 2011-06-28 20:29:54]

On 2011/06/28 10:33:31, Vyacheslav Egorov wrote:
> I think it's ready to land but I really want to exclude contents of samples
> directory for now. I strongly feel it does not belong there. Is it fine with
you
> Luke?
> 
> If it is fine please submit another CL with those files excluded and I'll land
> it. (tools/gdb-* is fine though).

No problem-- I'm happy that we're leaving the Python script in!

[Vyacheslav Egorov (Chromium), 2011-06-29 11:10:40]

Great. I'll be landing it then.

[Vyacheslav Egorov (Chromium), 2011-06-29 12:12:02]

I was doing some prelanding tests and found a couple of problems:


1) __function does not work for x64 at least on my simple test:

function g () { return 1; }
function f () {
  var ret = 0;
  for (var i = 1; i < 10000000; i++) {
    ret += g ();
  }
  return ret;
}
f()

set breakpoint in g with b g
v8print __function

Printed some SMI for me.

2) It's a bit unhandy that we declare locals to be some pointer sized structure.
User can't use normal print to print their values. See comment below. Is it
intentional?

3) I am a bit concerned that abbrev generation and DebugInfoSection are not
glued together. This is completely unrelated to this CL but I think if we start
putting more and more stuff there we'll suffer huge headache if we don't hide
them both under the same abstraction that writes both abbrevs and debug info
sections.

http://codereview.chromium.org/6995161/diff/6003/src/gdb-jit.cc
File src/gdb-jit.cc (right):

http://codereview.chromium.org/6995161/diff/6003/src/gdb-jit.cc#newcode1337
src/gdb-jit.cc:1337: w->WriteULEB128(DW_TAG_STRUCTURE_TYPE);
What about using DW_TAG_base_type with DW_ATE_address instead of
DW_TAG_STRUCTURE_TYPE?

I think this would allow GDB to produce something meaningfull when you say
"print __function" instead of just ().

It would be even better if we could hook type of locals to be
v8::internal::Object* but I can't grok how to do it just from DWARF2 spec. I'll
ask Paul whether this is possible.

[zarko, 2011-06-29 18:25:07]

On 2011/06/29 12:12:02, Vyacheslav Egorov wrote:
> I was doing some prelanding tests and found a couple of problems:
> 
> 
> 1) __function does not work for x64 at least on my simple test:
> 
> function g () { return 1; }
> function f () {
>   var ret = 0;
>   for (var i = 1; i < 10000000; i++) {
>     ret += g ();
>   }
>   return ret;
> }
> f()
> 
> set breakpoint in g with b g
> v8print __function
> 
> Printed some SMI for me.

That's pretty interesting; in ia32 this works fine, and if you drop out of the
function everything's OK too. The context appears to be correct in both
instances. Right now I am calculating the address of the function slot as:

        w->Write<uint8_t>(DW_OP_fbreg);
        w->WriteSLEB128(JavaScriptFrameConstants::kFunctionOffset);

(which on x64 turns into rbp-0x10)

and the context slot as:

        w->Write<uint8_t>(DW_OP_fbreg);
        w->WriteSLEB128(StandardFrameConstants::kContextOffset);

(which on x64 turns into rbp-0x8)

Is there some condition that would lead to the x64 code generator placing the
function somewhere else?

> 2) It's a bit unhandy that we declare locals to be some pointer sized
structure.
> User can't use normal print to print their values. See comment below. Is it
> intentional?

Yes, because otherwise gdb will ignore the pretty printer. I had a version
previously using DW_ATE_address and DW_TAG_base_type, but felt that it was
clearer if the user would see SMIs properly shifted and heap pointers
identified.

> 
> 3) I am a bit concerned that abbrev generation and DebugInfoSection are not
> glued together. This is completely unrelated to this CL but I think if we
start
> putting more and more stuff there we'll suffer huge headache if we don't hide
> them both under the same abstraction that writes both abbrevs and debug info
> sections.

Agreed; I'll look into ways to address this in a future CL.

> 
> http://codereview.chromium.org/6995161/diff/6003/src/gdb-jit.cc
> File src/gdb-jit.cc (right):
> 
> http://codereview.chromium.org/6995161/diff/6003/src/gdb-jit.cc#newcode1337
> src/gdb-jit.cc:1337: w->WriteULEB128(DW_TAG_STRUCTURE_TYPE);
> What about using DW_TAG_base_type with DW_ATE_address instead of
> DW_TAG_STRUCTURE_TYPE?
> 
> I think this would allow GDB to produce something meaningfull when you say
> "print __function" instead of just ().
> 
> It would be even better if we could hook type of locals to be
> v8::internal::Object* but I can't grok how to do it just from DWARF2 spec.
I'll
> ask Paul whether this is possible.

Is there a way to do this without depending on the way gcc mangles names (or is
that not an issue here)?