LGTM There is a lot of details in this CL, so I might have missed something.
A general coment is regarding using unsigned int for addresses. How about changing that to Address all over. I know class StackTracer already uses unsigned int, but that could be changed to Address as well. You could do that in a new CL. http://codereview.chromium.org/50052/diff/1/6 File src/frames.cc (right): http://codereview.chromium.org/50052/diff/1/6#newcode86 Line 86: if (use_top || fp != NULL) { Isn't fp != NULL sufficient? http://codereview.chromium.org/50052/diff/1/6#newcode208 Line 208: StackFrame* frame = iterator_.frame(); Maybe call this variable last_frame and prev_frame for frame. http://codereview.chromium.org/50052/diff/1/6#newcode222 Line 222: bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const { Add a comment on this check (JavaScript frame needs a valid function whereas other frames does not) http://codereview.chromium.org/50052/diff/1/6#newcode348 Line 348: (void)GetCallerState(state); Is the (void) cast required? http://codereview.chromium.org/50052/diff/1/7 File src/frames.h (right): http://codereview.chromium.org/50052/diff/1/7#newcode422 Line 422: inline Object* function_no_check() const; How about naming this function_slot_object instead? http://codereview.chromium.org/50052/diff/1/7#newcode523 Line 523: explicit StackFrameIterator(bool use_top, Address fp, Address sp); explicit not needed anymore. http://codereview.chromium.org/50052/diff/1/7#newcode545 Line 545: void (StackFrameIterator::*advance_)(void); (void) -> () http://codereview.chromium.org/50052/diff/1/7#newcode621 Line 621: Address low_bound, Address high_bound); explicit not required. http://codereview.chromium.org/50052/diff/1/8 File src/log.cc (right): http://codereview.chromium.org/50052/diff/1/8#newcode928 Line 928: msg.Append(",0x%x", reinterpret_cast<int>(sample->stack[i])); Change int to uint32_t. Btw. why does %p now work anymore? http://codereview.chromium.org/50052/diff/1/10 File src/platform-win32.cc (right): http://codereview.chromium.org/50052/diff/1/10#newcode1763 Line 1763: if (sampler_->IsProfiling()) { Good move! http://codereview.chromium.org/50052/diff/1/11 File test/cctest/test-log-ia32.cc (right): http://codereview.chromium.org/50052/diff/1/11#newcode4 Line 4: How about changing unsigned int to Address where it is an address in this file? Then use casting in the use of CHECK_GE macros, or add a CHECK_ADDR_GE macros. In test-debug.cc I have added helpers for the CHECK_EQ macros for Address, but these will not help as you need CHECK_GE unless we change these to call helpers as well, which might be overkill for this. http://codereview.chromium.org/50052/diff/1/11#newcode34 Line 34: StackTracer* tracer; Initialize tracer and sampler to NULL. http://codereview.chromium.org/50052/diff/1/11#newcode49 Line 49: reinterpret_cast<unsigned int>(trace_env.sample) - 10240; Why is 100 not enough any more? http://codereview.chromium.org/50052/diff/1/11#newcode210 Line 210: Please describe this function (e.g. create a JS function which calls another JS function with the current fp value). http://codereview.chromium.org/50052 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
