Basic GDB JIT Interface integration.

Basic GDB JIT Interface integration.

It has certain overheads even when gdb is not attached so it is guarded by ENABLE_GDBJIT_INTERFACE define and --gdbjit flag.

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

[Vyacheslav Egorov (Chromium), 2010-12-29 19:39:55]

Hi Mads,

This CL provides basic GDB JIT interface integration for the sake of V8
embedders who currently have to manually decode backtraces in gdb. 

Here is an example of current backtrace:

-------------------------------------------
(gdb) bt
#0  Print (args=...) at samples/shell.cc:159
#1  0x0807eff9 in v8::internal::HandleApiCallHelper<false> (args=...) at
src/builtins.cc:1058
#2  0x0807c908 in v8::internal::Builtin_Impl_HandleApiCall (args=...) at
src/builtins.cc:1075
#3  0x0807c8ee in v8::internal::Builtin_HandleApiCall (args=...) at
src/builtins.cc:1074
#4  0xf5cae28e in ?? ()
#5  0xf5cc37a4 in ?? ()
#6  0xf5cafc1f in ?? ()
#7  0xf5cc3741 in ?? ()
#8  0xf5cc36df in ?? ()
#9  0xf5cc367b in ?? ()
#10 0xf5cbebd9 in ?? ()
#11 0xf5cb1e42 in ?? ()
-------------------------------------------

And here is an example of backtrace with gdbjit interface integration enabled:

-------------------------------------------
(gdb) bt
#0  Print (args=...) at samples/shell.cc:159
#1  0x0807eff9 in v8::internal::HandleApiCallHelper<false> (args=...) at
src/builtins.cc:1058
#2  0x0807c908 in v8::internal::Builtin_Impl_HandleApiCall (args=...) at
src/builtins.cc:1075
#3  0x0807c8ee in v8::internal::Builtin_HandleApiCall (args=...) at
src/builtins.cc:1074
#4  0xf5cae28e in ?? ()
#5  0xf5cc4724 in baz () at test.js:10
#6  0xf5cafc1f in ?? ()
#7  0xf5cc46c1 in bar () at test.js:6
#8  0xf5cc465f in foo () at test.js:2
#9  0xf5cc45fb in test.js () at test.js:13
#10 0xf5cbebd9 in ?? ()
#11 0xf5cb1e42 in ?? ()

(gdb) f 5
#5  0xf5cc4724 in baz () at test.js:10
10        print("inside baz");
(gdb) f 8
#8  0xf5cc465f in foo () at test.js:2
2         bar();
-------------------------------------------

This CL is as unobtrusive as possible. All tests in testsuite pass with full
integration enabled.

Current limitations:
- Only ia32 supported. Certain changes in ELF object generation should be made
to enable it on x64.
-  Information emitted in DWARF2 .line_info section is minimal and does not
cover all code, so setting breakpoints by line numbers and steping through
generated code with gdb is not possible.
- No stack unwinding information is emitted. Thus no information about locals
and function arguments will be available in gdb for generated code.
- If V8 is compiled with snapshot no in memory ELF objects will be generated for
code de-serialized from snapshot. Thus currently backtrace for V8 without
snapshot is more informative:

-------------------------------------------
#4  0xf5cae06e in STUB: CEntryStub ()
#5  0xf5cc3084 in baz () at test.js:10
#6  0xf5cae7ff in BUILTIN: ArgumentsAdaptorTrampoline ()
#7  0xf5cee6a9 in ?? ()
#8  0xf5cc3021 in bar () at test.js:6
#9  0xf5cc2fbf in foo () at test.js:2
#10 0xf5cc2f5b in test.js () at test.js:13
#11 0xf5caee99 in BUILTIN: JSEntryTrampoline ()
#12 0xf5cee6a9 in ?? ()
#13 0xf5cae2c2 in STUB: JSEntryStub ()
-------------------------------------------

[ry, 2010-12-30 04:53:03]

Tested in Node (
https://github.com/ry/node/commit/3f45187815ba043ef04b591da53d6cacb11937c1 ) and
it seems to work. It's extremely slow though; like 5 minutes to start a hello
world http server. Is it possible to run on non-debug builds?

[Vyacheslav Egorov (Chromium), 2011-01-03 14:17:00]

Redirecting CL to Erik.

[Erik Corry, 2011-01-04 14:21:48]

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc
File src/gdbjit.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode95
src/gdbjit.cc:95: Slot<T> SlotHere() {
The name should reflect the fact that this function (and the next) create slots.
 eg CreateSlotHere()

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode102
src/gdbjit.cc:102: Ensure(position_ += sizeof(T) * count);
no no no no

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode311
src/gdbjit.cc:311: WriteString("");
I'm sure there is a good reason why we do this which we could put in a comment.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode401
src/gdbjit.cc:401: #endif
At least one place in this file should have an else and a #error to catch the
ARM case and give a sensible error message.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode858
src/gdbjit.cc:858: is_statement = !is_statement;
How about is_statement = info->is_statement_;

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h
File src/gdbjit.h (right):

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode31
src/gdbjit.h:31: #ifdef ENABLE_GDBJIT_INTERFACE
A few lines here on which gdb versions, OSs etc. this is expected to work on
would be nice.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode59
src/gdbjit.h:59: void SetPosition(intptr_t pc, int pos, bool is_statement) {
What is pos, a line number?

The names of the function and the variables should tell us more.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode72
src/gdbjit.h:72: List<PCInfo>* pc_info() {
Lower case names are reserved for getters.  Doing a sort means it is not just a
getter.  Either the function should be renamed or the sort should be moved out.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode82
src/gdbjit.h:82: return a->pc_ - b->pc_;
Subtracting two 64 bit values and returning the answer as a signed 32 bit value
is rather fragile even though it probably works right now.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode91
src/gdbjit.h:91: 
There should be two blank lines here.

[Erik Corry, 2011-01-05 09:00:15]

LGTM

http://codereview.chromium.org/5965011/diff/12016/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5965011/diff/12016/SConstruct#newcode882
SConstruct:882: Abort("GDBJIT interface is supported only for 32-bit Linux
target.")
32-bit should perhaps be Intel-compatible?

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h#newcode38
src/assembler.h:38: #include "gdbjit.h"
The files should really be called gdb-jit.*

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h#newcode637
src/assembler.h:637: #ifdef ENABLE_GDBJIT_INTERFACE
and the macro should really be called ENABLE_GDB_JIT_INTERFACE.

http://codereview.chromium.org/5965011/diff/12016/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/bootstrapper.cc#newcode36
src/bootstrapper.cc:36: #include "gdbjit.h"
This shouldn't be necessary.  The .cc file doesn't use it and the .h files
should include what they need.

http://codereview.chromium.org/5965011/diff/12016/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/builtins.cc#newcode1549
src/builtins.cc:1549: Object* code = 0;
Not your code, but this should be NULL.  Also it should be typed as a Code*
instead of being cast to it 3 places below.

http://codereview.chromium.org/5965011/diff/12016/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/mark-compact.cc#newcode131
src/mark-compact.cc:131: // If GDBJIT interface is active disable compaction.
We should remember to fix this in the new GC.  We only have to disable
compaction of code space or perhaps we can do it as a delete followed by an add,
though that may kill gdb breakpoints.  Perhaps this feature should be mentioned
somewhere prominent like the --help text.

[Vyacheslav Egorov (Chromium), 2011-01-18 16:10:42]

Thanks for the review Erik!

Landing.

I will also create a Wiki and Issues to cover current progress.

http://codereview.chromium.org/5965011/diff/12016/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5965011/diff/12016/SConstruct#newcode882
SConstruct:882: Abort("GDBJIT interface is supported only for 32-bit Linux
target.")
On 2011/01/05 09:00:15, Erik Corry wrote:
> 32-bit should perhaps be Intel-compatible?

Done.

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h#newcode38
src/assembler.h:38: #include "gdbjit.h"
On 2011/01/05 09:00:15, Erik Corry wrote:
> The files should really be called gdb-jit.*

Done.

http://codereview.chromium.org/5965011/diff/12016/src/assembler.h#newcode637
src/assembler.h:637: #ifdef ENABLE_GDBJIT_INTERFACE
On 2011/01/05 09:00:15, Erik Corry wrote:
> and the macro should really be called ENABLE_GDB_JIT_INTERFACE.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/bootstrapper.cc#newcode36
src/bootstrapper.cc:36: #include "gdbjit.h"
On 2011/01/05 09:00:15, Erik Corry wrote:
> This shouldn't be necessary.  The .cc file doesn't use it and the .h files
> should include what they need.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/builtins.cc#newcode1549
src/builtins.cc:1549: Object* code = 0;
On 2011/01/05 09:00:15, Erik Corry wrote:
> Not your code, but this should be NULL.  Also it should be typed as a Code*
> instead of being cast to it 3 places below.

We get it from ToObject() which does not provide get and case primitive.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc
File src/gdbjit.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode95
src/gdbjit.cc:95: Slot<T> SlotHere() {
On 2011/01/04 14:21:48, Erik Corry wrote:
> The name should reflect the fact that this function (and the next) create
slots.
>  eg CreateSlotHere()

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode102
src/gdbjit.cc:102: Ensure(position_ += sizeof(T) * count);
On 2011/01/04 14:21:48, Erik Corry wrote:
> no no no no

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode311
src/gdbjit.cc:311: WriteString("");
On 2011/01/04 14:21:48, Erik Corry wrote:
> I'm sure there is a good reason why we do this which we could put in a
comment.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode401
src/gdbjit.cc:401: #endif
On 2011/01/04 14:21:48, Erik Corry wrote:
> At least one place in this file should have an else and a #error to catch the
> ARM case and give a sensible error message.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode858
src/gdbjit.cc:858: is_statement = !is_statement;
On 2011/01/04 14:21:48, Erik Corry wrote:
> How about is_statement = info->is_statement_;

I want to reflect the meaning of an emitted operation: in this case emitted
operation is negation not an assignment.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h
File src/gdbjit.h (right):

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode31
src/gdbjit.h:31: #ifdef ENABLE_GDBJIT_INTERFACE
On 2011/01/04 14:21:48, Erik Corry wrote:
> A few lines here on which gdb versions, OSs etc. this is expected to work on
> would be nice.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode59
src/gdbjit.h:59: void SetPosition(intptr_t pc, int pos, bool is_statement) {
On 2011/01/04 14:21:48, Erik Corry wrote:
> What is pos, a line number?
> 
> The names of the function and the variables should tell us more.

pos is used in common V8 sense --- position in string.

it is not line number.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode72
src/gdbjit.h:72: List<PCInfo>* pc_info() {
On 2011/01/04 14:21:48, Erik Corry wrote:
> Lower case names are reserved for getters.  Doing a sort means it is not just
a
> getter.  Either the function should be renamed or the sort should be moved
out.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode82
src/gdbjit.h:82: return a->pc_ - b->pc_;
On 2011/01/04 14:21:48, Erik Corry wrote:
> Subtracting two 64 bit values and returning the answer as a signed 32 bit
value
> is rather fragile even though it probably works right now.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode91
src/gdbjit.h:91: 
On 2011/01/04 14:21:48, Erik Corry wrote:
> There should be two blank lines here.

Done.

http://codereview.chromium.org/5965011/diff/12016/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/5965011/diff/12016/src/mark-compact.cc#newcode131
src/mark-compact.cc:131: // If GDBJIT interface is active disable compaction.
On 2011/01/05 09:00:15, Erik Corry wrote:
> We should remember to fix this in the new GC.  We only have to disable
> compaction of code space or perhaps we can do it as a delete followed by an
add,
> though that may kill gdb breakpoints.  Perhaps this feature should be
mentioned
> somewhere prominent like the --help text.

Done.