Hi Vyacheslav. I'll make the changes you suggested. Below are some comments, however, regarding your comments:
- Shasank On 2/1/11 4:49 AM, "[email protected]" <[email protected]> wrote: > 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? >>>> Yes, I did ;). We build outside of scons (when targeting ARM), and that's why I forgot - I'll correct this. > 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. >>>> Right - I put it there as a point that two statements on the same line could be differentiated if we tracked column positions. But I'll rewrite it to just mention "line". > 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). >>>> Thanks. For some reason I thought this was just an accessor function (i.e. O(1)). > 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. >>>> Will correct. Thanks. > 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. >>>> Right. I didn't want to reduce the code until it was understood. I'll optimize 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. >>>> That's right - see my comment above. > 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. >>>> Okay. > 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). >>>> Okay. > 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. >>>> You're right. Thanks. > 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. > >>>> Will do. Thanks. > 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/ >>>> If I add the space, the line will be longer than 80 bytes ;). I guess I can split the call into two lines, separating out the arguments to Lookup(). I'll do that if you like. Thanks. - Shasank -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
