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#newcode2999
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#newcode72
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#newcode67
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.
http://codereview.chromium.org/6371011/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
