Hi Shasank, Thanks for fixing this.
Please see review comments. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc File src/gdb-jit.cc (right): http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode397 src/gdb-jit.cc:397: #if defined(V8_TARGET_ARCH_IA32) || defined(V8_TARGET_ARCH_ARM) I think that you need to change SConstruct to enable building with gdbjit=on on arm but I do not see it in this changelist. Did you accidentally omit it? http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode866 src/gdb-jit.cc:866: // <file, line, column> entries (per dwarf 2.0 standard). Comment is slightly confusing as we do not track column information. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode890 src/gdb-jit.cc:890: intptr_t line_diff = desc_->GetScriptLineNumber(info->pos_) - line; I think desc_->GetScriptLineNumber(info->pos_) is already in new_line variable. You can use it here to avoid recalculation (it has linear complexity, not O(1) one AFAIK). http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode896 src/gdb-jit.cc:896: // If the special opcode is < 255, it can be used as a special opcode. I prefer to write "less than" instead of "<". BTW you probably meant less than or equal. This sentence is also slightly confusing: can you please change first "special opcode" to special_opcode. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode910 src/gdb-jit.cc:910: if (line_diff != 0) { line_diff is guaranteed to be non-zero now. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode922 src/gdb-jit.cc:922: if (!special_opcode_used && (line_diff != 0)) { line_diff is guaranteed to be non-zero now. So you can move DW_LNS_COPY emittance to the end of the else branch and remove special_opcode_used variable. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode926 src/gdb-jit.cc:926: // Need to advance the pc to the end of the routine before an end sequence Empty line before comment. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode1046 src/gdb-jit.cc:1046: fwrite(entry->symfile_addr_, entry->symfile_size_, 1, file); I think you can use WriteBytes() utility function (see v8utils.h). http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode1060 src/gdb-jit.cc:1060: fileNum++; it's not clear why you close the file after calling __jit_debug_register_code(). I think code will be much more compact and readable if you close the file immediately after fwrite. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode1115 src/gdb-jit.cc:1115: static HashMap entries(&SameCodeObjects); Google C++ Style Guide explicitly forbid non POD static variable (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static_and_Global_Variables#Static_and_Global_Variables) and recommends to use pointers. We somehow managed to overlook this point. Can you change GetEntries so it will use pointer? Something like: static HashMap* GetEntries() { if (entries == NULL) { entries = new HashMap(&SameCodeObjects); } return entries; } should be fine. http://codereview.chromium.org/6287015/diff/4001/src/gdb-jit.cc#newcode1234 src/gdb-jit.cc:1234: HashMap::Entry* e =GetEntries()->Lookup(code, HashForCodeObject(code), false); after = add space http://codereview.chromium.org/6287015/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
