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