Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276813002:
Unwind stack past system libraries on Linux. (Closed)
|
Created:
4 years, 4 months ago by Dmitry Skiba Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@get-stack-end-linux Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnwind stack past system libraries on Linux.
Native heap profiler traces on Linux are sometimes incomplete because unwinder
hits a function from a system library (i.e. no frame pointers), finds bad stack
frame and stops.
This CL implements stack scanning, so instead of stopping unwinder scans stack
for a valid frame pointer and resumes unwinding.
BUG=624362
Committed: https://crrev.com/678c7c2a3ee8a55c7a5c532e46abf94bf48858cd
Cr-Commit-Position: refs/heads/master@{#417343}
Patch Set 1 #Patch Set 2 : Fix comment #
Total comments: 10
Patch Set 3 : Introduce ScanStackForNextFrame #
Total comments: 5
Patch Set 4 : Address comments; bump area to 8192 #Messages
Total messages: 14 (4 generated)
dskiba@google.com changed reviewers: + primiano@chromium.org, thakis@chromium.org
PTAL
Cool, I feel the new helper functions help to read better the existing code. I've some comments below, mostly stylistic to avoid the extra macro and making the code more readable. Not sure I follow the 2nd if in the stack scanning algorithm. See comments inline. Thanks a lot for fixing this. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:209: if (!IsStackFrameValid(next_fp, fp, stack_end)) { I'd reverse the if condition and reduce one leven of indentation here, I think makes the overall code more readable if you do: if (!IsStackFrameValid(next_fp, fp, stack_end)) continue; // Could not reconstruct a frame using frame pointer. Resorting on // stack scanning. ... https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:210: #if defined(SCAN_STACK_FOR_FRAMES) Since you introduced all these beautiful helper functions here (nice), maybe you can also have another helper for the stack scan. The would remove the need of this SCAN_STACK_FOR_FRAME macro as you can: 1. here just do: next_fp = ScanStackForNextFrame(...) if (!next_fp) break; 2. above you can: uintptr_t ScanStackForNextFrame(...) { #if OS_LINUX... ... # else return 0; # } Which IMHO reduces the need of jumping up and down with the eyes to follow the macros. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:215: uintptr_t last_scan_fp = std::min(fp + kMaxStackScanArea, stack_end) - Just a minor naming thing: when I saw "last_scan_fp" for a while I thought this was "the resulting fp of the last stack scan". Took me a while to realize that this is "the end boundary where to stop the current stack scanning". Maybe call it stack_scan_boundary ? or stack_scan_end? or stack_scan_limit? I'd avoid "last", as IMHO can mistakenly get confused with "previous iteration" https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:224: uintptr_t next2_fp = GetNextStackFrame(next_fp); Hmm I'm missing something here, does this mean that you stack scan works only if you have exactly one frame missing a FP? If I read the code correctly this is: - looking for something that looks like a pointer and checking if it's withing the stack. - If so, you find a very likely candidate. So far so good. But now in the second nested if() you expect that the frame you found has a valid FP. This might not be the case if you have two or more functions that don't have a valid FP. Or am I misreading completely this?
https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:209: if (!IsStackFrameValid(next_fp, fp, stack_end)) { On 2016/08/25 11:42:53, Primiano Tucci wrote: > I'd reverse the if condition and reduce one leven of indentation here, I think > makes the overall code more readable if you do: > > if (!IsStackFrameValid(next_fp, fp, stack_end)) > continue; > > // Could not reconstruct a frame using frame pointer. Resorting on > // stack scanning. > ... Done. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:210: #if defined(SCAN_STACK_FOR_FRAMES) On 2016/08/25 11:42:53, Primiano Tucci wrote: > Since you introduced all these beautiful helper functions here (nice), maybe you > can also have another helper for the stack scan. > The would remove the need of this SCAN_STACK_FOR_FRAME macro as you can: > 1. here just do: > next_fp = ScanStackForNextFrame(...) > if (!next_fp) break; > > 2. above you can: > uintptr_t ScanStackForNextFrame(...) { > #if OS_LINUX... > ... > # else > return 0; > # > } > > Which IMHO reduces the need of jumping up and down with the eyes to follow the > macros. Done. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:215: uintptr_t last_scan_fp = std::min(fp + kMaxStackScanArea, stack_end) - On 2016/08/25 11:42:53, Primiano Tucci wrote: > Just a minor naming thing: when I saw "last_scan_fp" for a while I thought this > was "the resulting fp of the last stack scan". > Took me a while to realize that this is "the end boundary where to stop the > current stack scanning". > Maybe call it stack_scan_boundary ? or stack_scan_end? or stack_scan_limit? I'd > avoid "last", as IMHO can mistakenly get confused with "previous iteration" Done. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:224: uintptr_t next2_fp = GetNextStackFrame(next_fp); On 2016/08/25 11:42:53, Primiano Tucci wrote: > Hmm I'm missing something here, does this mean that you stack scan works only if > you have exactly one frame missing a FP? > If I read the code correctly this is: > - looking for something that looks like a pointer and checking if it's withing > the stack. > - If so, you find a very likely candidate. So far so good. > But now in the second nested if() you expect that the frame you found has a > valid FP. This might not be the case if you have two or more functions that > don't have a valid FP. > > Or am I misreading completely this? This tries to get rid of false positives - it's fairly easy to find something on the stack that points further to the stack, and mistake that for a frame pointer. Nested if() requires found frame pointer to be linked to the next valid frame, reducing chances of finding garbage.
LGTM with some final comments. Mind the check on the first iteration you just lost :) https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:210: #if defined(SCAN_STACK_FOR_FRAMES) On 2016/08/30 18:04:21, Dmitry Skiba wrote: > On 2016/08/25 11:42:53, Primiano Tucci wrote: > > Since you introduced all these beautiful helper functions here (nice), maybe > you > > can also have another helper for the stack scan. > > The would remove the need of this SCAN_STACK_FOR_FRAME macro as you can: > > 1. here just do: > > next_fp = ScanStackForNextFrame(...) > > if (!next_fp) break; > > > > 2. above you can: > > uintptr_t ScanStackForNextFrame(...) { > > #if OS_LINUX... > > ... > > # else > > return 0; > > # > > } > > > > Which IMHO reduces the need of jumping up and down with the eyes to follow the > > macros. > > Done. Doesn't seem so :) WHat I mean is for ScanStackForNextFrame to be always defined. SO you don't need the SCAN_STACK_FOR_FRAME define and just make the entire implementation a noop if ! OS_LINUX. https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.... base/debug/stack_trace.cc:146: #if defined(SCAN_STACK_FOR_FRAMES) why this extra check is if-defed? makes sense in any case right? https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.... base/debug/stack_trace.cc:169: if (IsStackFrameValid(next_fp, fp, stack_end)) { Ahhh now I understand what this really does. For a moment I thought that you were trying to reconstruct the call stack even for frames that don't have a frame pointer, by inferring that an address on the stack is actually the beginning of a frame (without a valid fp). This is what breakpad does (but then you need to be more careful and intersecting the addresses with the mmaps to figure out which addresses lay in .text sections... the relative offset of fp and PC depend on the architecture... let's not go there) Now I understand that your intend here is to completely skip all the frames that do not have a frame pointer, and just recover to a known state, hoping that something up there in the stack has a frame pointer again. Makes perfect sense, maybe we should clarify in a comment that ScanStackForNextFrame is aimed at *skipping* all the frames that don't have a valid FP and just recovering the FP chain. https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.... base/debug/stack_trace.cc:197: StackTrace::~StackTrace() { nit: I think this should be {} (without the newline between braces) (make sure this is git-cl format-ed) https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.... base/debug/stack_trace.cc:233: out_trace[depth++] = reinterpret_cast<const void*>(GetStackFramePC(fp)); I think you just lost the protection for the first iteration. If what __builtin_frame_address(0) returned is invalid (you don't have a valid FP) this will just crash while dereferencing a null ptr. I think you need some check on fp vs stack_end before entering the while loop.
rs-lgtm based on primiano's review
Note that I'm bumping scan area to 8192 - that allows to resume more traces than previous value of 512 and didn't cause noticeable perf regression. See the bug for measurements. https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:210: #if defined(SCAN_STACK_FOR_FRAMES) On 2016/08/31 13:48:49, Primiano Tucci wrote: > On 2016/08/30 18:04:21, Dmitry Skiba wrote: > > On 2016/08/25 11:42:53, Primiano Tucci wrote: > > > Since you introduced all these beautiful helper functions here (nice), maybe > > you > > > can also have another helper for the stack scan. > > > The would remove the need of this SCAN_STACK_FOR_FRAME macro as you can: > > > 1. here just do: > > > next_fp = ScanStackForNextFrame(...) > > > if (!next_fp) break; > > > > > > 2. above you can: > > > uintptr_t ScanStackForNextFrame(...) { > > > #if OS_LINUX... > > > ... > > > # else > > > return 0; > > > # > > > } > > > > > > Which IMHO reduces the need of jumping up and down with the eyes to follow > the > > > macros. > > > > Done. > > Doesn't seem so :) > WHat I mean is for ScanStackForNextFrame to be always defined. SO you don't need > the SCAN_STACK_FOR_FRAME define and just make the entire implementation a noop > if ! OS_LINUX. Ah, I see. Done now :) https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2276813002/diff/40001/base/debug/stack_trace.... base/debug/stack_trace.cc:233: out_trace[depth++] = reinterpret_cast<const void*>(GetStackFramePC(fp)); On 2016/08/31 13:48:50, Primiano Tucci wrote: > I think you just lost the protection for the first iteration. > If what __builtin_frame_address(0) returned is invalid (you don't have a valid > FP) this will just crash while dereferencing a null ptr. > I think you need some check on fp vs stack_end before entering the while loop. Actually, __builtin_frame_address() will force frame pointers in the function that uses it - this is how unittest for TraceStackFramePointers() works even though base_unittests is not built with FP enabled - we're unwinding through a call chain where each function forces FP with __builtin_frame_address(). So here 'fp' will always be valid and will point to the frame that was created by TraceStackFramePointers() itself. GetNextStackFrame(fp) might not be valid, but that is handled.
PS3 still LGTM
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2276813002/#ps60001 (title: "Address comments; bump area to 8192")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Unwind stack past system libraries on Linux. Native heap profiler traces on Linux are sometimes incomplete because unwinder hits a function from a system library (i.e. no frame pointers), finds bad stack frame and stops. This CL implements stack scanning, so instead of stopping unwinder scans stack for a valid frame pointer and resumes unwinding. BUG=624362 ========== to ========== Unwind stack past system libraries on Linux. Native heap profiler traces on Linux are sometimes incomplete because unwinder hits a function from a system library (i.e. no frame pointers), finds bad stack frame and stops. This CL implements stack scanning, so instead of stopping unwinder scans stack for a valid frame pointer and resumes unwinding. BUG=624362 Committed: https://crrev.com/678c7c2a3ee8a55c7a5c532e46abf94bf48858cd Cr-Commit-Position: refs/heads/master@{#417343} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/678c7c2a3ee8a55c7a5c532e46abf94bf48858cd Cr-Commit-Position: refs/heads/master@{#417343} |
