Ensures that GDB prints stacktraces correctly for x64 builds when debugging t...

Ensures that GDB prints stacktraces correctly for x64 builds when debugging the host application architectures by emitting an appropriate .eh_frame section.

[Vyacheslav Egorov (Chromium), 2011-01-25 12:50:11]

Good start.

He are the main points I am concerned about (they will be repeated several times
in the comments, sorry about that):

1) I really don't think we should emit and register .eh_frame as a separate code
entry and handle it in a special way like you do in builtins.cc/code-stubs.cc
(see also next point).

There are several reasons for that:

a) Currently GDB becomes slower and slower with each registered code object.

b) You have a leak: when code object is destroyed you do not unregister and
deallocate code entry emitted for this code object. 

c) I think the code that handles unwind info is a bit too complicated with all
this chaining and magical constructors and destructor. When we register stub or
builtin code object with GDBJIT we already pass appropriate CodeTag. It can tell
you what kind of unwinding information to emit.

2) We want GDBJIT to be as unobtrusive as possible. It is highly unlikely that
code emitted for prologue and epilogue of functions compiled by full- will
change in the future so it seems reasonable to me use fixed offsets (from the
begging for prologue, from the end for epilogue) when emitting unwind
information. The other option is to walk instruction stream from the start to
find push ebp (prologue start) and walk it from the end to find ret (epilogue
end). 

I think removing special handling of .eh_frame and emitting it where all other
sections are emitted based on fixed offsets will greatly simplify and clean the
code.

I am open to suggestions, we can discuss it over IRC.

3) I am considering dropping support for GDBJIT in the classic codegen (and
turning --always-full-compiler by default if user turned --gdbjit). Classic
codegen is going to be replaced by Crankshaft codegen in the near future (it is
already replaced on ia32) so supporting anything in the classic codegen is just
a waste of time.

Thanks for implementing this!

http://codereview.chromium.org/6371011/diff/1010/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/builtins.cc#newcode1498
src/builtins.cc:1498: ZoneScope uinfo(DELETE_ON_EXIT);
Do we really need this piece of code?

The code stub was registered above as GDBJITInterface::CodeTag::BUILTIN so
GDBJIT really knows how to handle it.

http://codereview.chromium.org/6371011/diff/1010/src/code-stubs.cc
File src/code-stubs.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/code-stubs.cc#newcode112
src/code-stubs.cc:112: {
Do we really need this piece of code? 

The code stub will be later registered as GDBJITInterface::CodeTag::STUB so at
the time of registration we will be able to say that this is STUB and handle
it's unwinding information accordingly.

http://codereview.chromium.org/6371011/diff/1010/src/codegen.cc
File src/codegen.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/codegen.cc#newcode253
src/codegen.cc:253: #ifdef V8_TARGET_ARCH_X64
Revert this change.

I am considering disabling GDBJIT support in classic codegen completely.

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

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1176
src/gdb-jit.cc:1176: explicit UnwindInfoSection(uintptr_t begin, uintptr_t end);
2 argument constructors don't need explicit.

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1181
src/gdb-jit.cc:1181: uintptr_t begin_, length_;
uintptr_t begin_;
uintptr_t length_;

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1186
src/gdb-jit.cc:1186: DW_CFA_ADVANCE_LOC = 0X40,
style nit: 0X -> 0x

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1233
src/gdb-jit.cc:1233:
UnwindInfoSection::UnwindInfoSection(UnwindInfoInterface::UnwindInformation
I think the right way to format this will be 

UnwindInfoSection::UnwindInfoSection(
  UnwindInfoInterface::UnwindInformation* info)

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1235
src/gdb-jit.cc:1235: : ELFSection(".eh_frame", TYPE_X86_64_UNWIND, 1),
info_(info),
either all on a single line or each initializer on it's own line

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1240
src/gdb-jit.cc:1240: : ELFSection(".eh_frame", TYPE_X86_64_UNWIND, 1),
begin_(begin),
ditto

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1259
src/gdb-jit.cc:1259: if (align) {
We don't rely on implicit int->bool convertions, so instead of if (align) -> if
(align == 0)

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1282
src/gdb-jit.cc:1282: // -- The first state, just after the control has been
transferred to the
Can you split this into four different functions for each state?

Also remove -- from the comments.

http://codereview.chromium.org/6371011/diff/1010/src/gdb-jit.cc#newcode1384
src/gdb-jit.cc:1384: if ((align = (w->position() - fde_position) %
kPointerSize)) {
I think we usually avoid assignments in expressions. So

align = ...
if (...)

Also I see code duplication. Does it make sense to create a separate function?

http://codereview.chromium.org/6371011/diff/1010/src/x64/codegen-x64.cc
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/x64/codegen-x64.cc#newco...
src/x64/codegen-x64.cc:2999: 
Revert this change.

Classic codegen is going to be replaced by Crankshaft in the future so we think
there is no need to support GDBJIT in classic codegen.

People should run with --always-full-compiler if they want --gdbjit (or even
--gdbjit will enable --always-full-compiler by default).

There is some support for line information in codegen.cc that I added but I
think I will just revert that.

http://codereview.chromium.org/6371011/diff/1010/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:72: SET_UNWIND_INFO(UNWIND_STATE_AFTER_RBP_PUSH,
masm_->pc_offset());
The idea is to keep GDBJIT as isolated as possible we do not want to obscure
codegeneration with calls to GDBJIT if it is possible.

PC offsets for UNWIND_STATE will almost fixed for all functions generated by
fullcodegen. So it seems possible to move this completely to gdb-jit.cc.

http://codereview.chromium.org/6371011/diff/1010/src/x64/virtual-frame-x64.cc
File src/x64/virtual-frame-x64.cc (right):

http://codereview.chromium.org/6371011/diff/1010/src/x64/virtual-frame-x64.cc...
src/x64/virtual-frame-x64.cc:67: SET_UNWIND_INFO(UNWIND_STATE_AFTER_RBP_PUSH,
masm()->pc_offset());
The classic codegen is going to be replaced with Crankshaft codegen in the
future.

We think there is no need to change it.

People can run with --always-full-compiler to get debugging information for all
code.

[Vyacheslav Egorov (Chromium), 2011-02-01 13:17:24]

Hi Sanjoy,

We are almost at the landing point.

I just need you to check a couple of questions and style nitpicks.

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc
File src/gdb-jit.cc (left):

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#oldcode1091
src/gdb-jit.cc:1091: if (!FLAG_gdbjit_full &&
!code_desc.is_line_info_available()) {
Why did you kill FLAG_gdbjit_full support?

It is an optimization: without it simple node.js application takes several
minutes to start under GDB.

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

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode657
src/gdb-jit.cc:657: GDBJITInterface::CodeTag type)
nitpick: type -> tag

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode662
src/gdb-jit.cc:662: type_(type)
nitpick: type -> tag

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode690
src/gdb-jit.cc:690: ASSERT(ofs < OFFSET_TYPE_MAX);
will not compile on ia32

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode695
src/gdb-jit.cc:695: ASSERT(ofs < OFFSET_TYPE_MAX);
will not compile on ia32

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode709
src/gdb-jit.cc:709: GDBJITInterface::CodeTag get_type() const {
nitpick: type -> tag

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode710
src/gdb-jit.cc:710: return type_;
nitpick: type -> tag

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode718
src/gdb-jit.cc:718: GDBJITInterface::CodeTag type_;
nitpick: type -> tag

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode1164
src/gdb-jit.cc:1164: elf->AddSection(new UnwindInfoSection(desc));
I don't think it will compile on ia32

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode1332
src/gdb-jit.cc:1332: #define RBP_PUSH_OFFSET 1
We prefer constants to macroses:

static const int kRBPPushOffset = 1;

etc.

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode1351
src/gdb-jit.cc:1351: #define AddUnwindInfo(x) ((void) x)
Move #ifdef into function body to avoid defining macros.

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.cc#newcode1365
src/gdb-jit.cc:1365: if (e->value != NULL && !IsLineInfoTagged(e->value))
Is there a reason for this change?

If we are registering several code object twice we will leak memory here.

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.h
File src/gdb-jit.h (right):

http://codereview.chromium.org/6371011/diff/35001/src/gdb-jit.h#newcode113
src/gdb-jit.h:113: CodeTag type,
nitpick: type -> tag

[Vyacheslav Egorov (Chromium), 2011-02-01 21:00:25]

LGTM (with minor nitpicks)

I will fix those nitpicks myself and land the patch. 

Thanks for contributing!

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

http://codereview.chromium.org/6371011/diff/38001/src/gdb-jit.cc#newcode711
src/gdb-jit.cc:711: GDBJITInterface::CodeTag get_tag() const {
Getters don't have get_ prefix in Google C++ Code Style.

http://codereview.chromium.org/6371011/diff/38001/src/gdb-jit.cc#newcode1061
src/gdb-jit.cc:1061: WriteFDEStateOne(w);
This names are not informative.

http://codereview.chromium.org/6371011/diff/38001/src/gdb-jit.cc#newcode1341
src/gdb-jit.cc:1341: 
This does not lint (did you run tools/presubmit.py?)

http://codereview.chromium.org/6371011/diff/38001/src/gdb-jit.cc#newcode1344
src/gdb-jit.cc:1344: byte *code = reinterpret_cast<byte*>(cd->code_start());
This will fail to compile in release mode (due to unused variable).

[Vyacheslav Egorov (Chromium), 2011-02-02 13:33:57]

I've landed your patch: http://code.google.com/p/v8/source/detail?r=6575 with
some renaming/cleanup.

Thanks. Closing this issue.