Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1879073002:
Add function to trace stack using frame pointers. (Closed)
|
Created:
4 years, 8 months ago by Dmitry Skiba Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd function to trace stack using frame pointers.
For memory-infra we need fast stack traces to implement allocation
tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding
and is too slow. This change adds a function that uses frame pointers
to walk the stack.
The function supports x86, x64 and arm (but not thumb) architectures
on POSIX platforms.
BUG=602701
Committed: https://crrev.com/ead0969fe721b3a71a18abb9c1203c15e1276aa4
Cr-Commit-Position: refs/heads/master@{#389123}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Total comments: 15
Patch Set 3 : rebase #Patch Set 4 : Add comments; fix tests #Patch Set 5 : Revert buildfile changes #Patch Set 6 : Force frame pointers in test functions #Patch Set 7 : Support Android and ARM #Patch Set 8 : Properly exclude Thumb; saner EXPECT_ macro to force frame pointers #
Dependent Patchsets: Messages
Total messages: 41 (8 generated)
dskiba@google.com changed reviewers: + thakis@chromium.org
thakis@chromium.org changed reviewers: + bcwhite@chromium.org
I think bcwhite has been also looking at manual stack walking (on windows iirc?) elsewhere, maybe he can have a first look?
I'm afraid my stack knowledge is limited to looking through minidump files on Windows (with it doing most of the work). https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace.h File base/debug/stack_trace.h (right): https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace.h#ne... base/debug/stack_trace.h:115: size_t skip_count); skip_initial? https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace_posi... File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace_posi... base/debug/stack_trace_posix.cc:777: // On 64-bit machines, the stack pointer can be very close to You say 64-bit but have 32-bit numbers. https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace_posi... base/debug/stack_trace_posix.cc:779: // last two pages in the address fpace What is special about the last two pages? Should this be checked before the first fp is dereferenced on line 764?
Guys, test that I added is failing on some bots - it's only tracing one frame, instead of 10. I suspect that frame pointers are disabled in those configurations. So I'm thinking of forcefully enabling frame pointers for base_unittests, creating a local non-inlineable chain of 5 functions in stack_trace_unittest.cc, and comparing only those 5 frames. What do you think? Maybe there are some other ways to test, or maybe we don't need to test this at all? https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace_posi... File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/1879073002/diff/1/base/debug/stack_trace_posi... base/debug/stack_trace_posix.cc:777: // On 64-bit machines, the stack pointer can be very close to On 2016/04/13 12:36:24, bcwhite wrote: > You say 64-bit but have 32-bit numbers. This code came from TCMalloc's stacktrace_x86-inl.h, and it appears to be very old - it was there in 2005. So I guess I'll just remove this hack.
Sorry, dropped the ball here. I have a general question: We don't really need 32-bit x86 anymore, and on 64-bit x86 I think the abi guarantees having unwind tables in the executable. Is reading those (which _Unwind_backtrace does when they're there) too slow? Do you think you'll get useful data from this? You won't see functions that have been inlined, tail-call eliminated, ICF'd, etc. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.h File base/debug/stack_trace.h (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.h:25: #if defined(OS_POSIX) && (defined(__i386__) || defined(__x86_64__)) do we still have any 32-bit posix platforms we need this on?
Also, do you have rough numbers for "too slow"?
Frame pointers are omitted in release builds except when using profiling (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...).
On 2016/04/15 20:01:46, Nico wrote: > Frame pointers are omitted in release builds except when using profiling > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...). Hmm, this points to -fno-omit-frame-pointers, which actually prevents frame pointers from being omitted.
So in case of native allocation tracing, we take stack trace for each allocation, and tracing using frame pointers is faster. I try to get numbers. The code should work for x86 unmodified, so even if we don't have a usecase I don't think we need to artificially limit our scope.
On 2016/04/15 20:31:43, Dmitry Skiba wrote: > So in case of native allocation tracing, we take stack trace for each > allocation, and tracing using frame pointers is faster. I try to get numbers. StackTrace (i.e. backtrace()) is much much slower on Linux: 10 iterations: StackTrace took 84 us, while my function took 2 us 1000 iterations: 7755 vs 18 TraceStackFramePointers returns less frames: #0 0x00000052f230 base::debug::StackTraceTest_TraceStackFramePointers_Test::TestBody() #1 0x000000cd76ba testing::Test::Run() #2 0x000000cd8389 testing::TestInfo::Run() #3 0x000000cd8823 testing::TestCase::Run() #4 0x000000cdfa29 testing::internal::UnitTestImpl::RunAllTests() #5 0x000000cdf6d5 testing::UnitTest::Run() #6 0x000000ae77ad base::TestSuite::Run() #7 0x000000afb3d5 base::(anonymous namespace)::LaunchUnitTestsInternal() #8 0x000000afb289 base::LaunchUnitTests() #9 0x000000adf035 main #10 0x7f7ea98e3ec5 __libc_start_main compared to StackFrames' #0 0x000000c41113 base::debug::StackTrace::StackTrace() #1 0x00000052f2a9 base::debug::StackTraceTest_TraceStackFramePointers_Test::TestBody() #2 0x000000cd76ca testing::Test::Run() #3 0x000000cd8399 testing::TestInfo::Run() #4 0x000000cd8833 testing::TestCase::Run() #5 0x000000cdfa39 testing::internal::UnitTestImpl::RunAllTests() #6 0x000000cdf6e5 testing::UnitTest::Run() #7 0x000000ae77bd base::TestSuite::Run() #8 0x000000afb3e5 base::(anonymous namespace)::LaunchUnitTestsInternal() #9 0x000000afb299 base::LaunchUnitTests() #10 0x000000adf045 main #11 0x7fea9d494ec5 __libc_start_main #12 0x0000004a717d <unknown> but still, it's crazy how StackTrace is slow. What is interesting, is that on MacOS, where backtrace also uses frame pointers (see http://stackoverflow.com/questions/27955072/how-does-os-x-implement-backtrace) my version compiled as part of base_unittests in Release configuration is two times slower for 1000 iterations: TraceStackFramePointers got 20 frames in 142 us StackTrace got 23 frames in 72 us
On 2016/04/15 20:31:35, Dmitry Skiba wrote: > On 2016/04/15 20:01:46, Nico wrote: > > Frame pointers are omitted in release builds except when using profiling > > > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...). > > Hmm, this points to -fno-omit-frame-pointers, which actually prevents frame > pointers from being omitted. Yes, that's what I tried to say with "Frame pointers are omitted in release builds except when using profiling" :-) lgtm, minor comments below. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:751: uintptr_t fp = reinterpret_cast<uintptr_t>(__builtin_frame_address(0)); nit: call this sp https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:758: out_trace[depth++] = reinterpret_cast<const void**>(fp)[1]; you treat fp as void** almost everywhere, consider making it type void** instead of uintptr_t
On 2016/04/18 18:04:26, Dmitry Skiba wrote: > On 2016/04/15 20:31:43, Dmitry Skiba wrote: > > So in case of native allocation tracing, we take stack trace for each > > allocation, and tracing using frame pointers is faster. I try to get numbers. > > StackTrace (i.e. backtrace()) is much much slower on Linux: > > 10 iterations: StackTrace took 84 us, while my function took 2 us > 1000 iterations: 7755 vs 18 I know that it's easy to say after the fact, but I was expecting this :) CFI-based unwinding can be very complex. You need to jump (i.e. page-fault) back and forth between the stack and the CFI data. On top of that CFI needs to reconstruct the full frame context just to figure out the beginning of the stack. In turn, on some modern archs (x86_64 and arm64) that implies feeding the stack to a rather complex FSM to unroll the context of the frame (having said that I never understood how much of that DWARD CFI is ABI-mandated, compiler/DWARD mandated or whatnot) On the other side walking the FP chain is as easy as walking a linked list which very likely all in L2 cache. * Other fun things about FPO: - On IOS we keep frame pointers, in crbug.com/601759 I've been told that DWARF CFI is not a thing there. - On Android arm64 we keep frame pointers. Initially rmcilroy@ and I thought it was required to properly unwind frames coming from libc (crbug.com/391706) but lately it seems not really required and the real problem is that our (breakpad) frame pointer unwinder is slightly broken crbug.com/601759. - On Windows we keep frame pointers, as per 2012 (https://chromium.googlesource.com/chromium/src/+/65d61e68a0a01327d5e2e34e5490...) - On Android arm would be nice to keep frame pointers as that would make the debuggerd output meaningful and not require to rely only on microdumps. But dskiba@ tried that and seems to have some visible perf impact. * to be really a perf freak this could be maybe further optimized speculatively firing a prefetchnta /__builtin_prefetch or similar, but literature is very controversial about prefetch so let's not really go there, it's more an intellectual curiosity (https://lwn.net/Articles/444336/, http://yarchive.net/comp/linux/software_prefetching.html)
primiano@chromium.org changed reviewers: + primiano@chromium.org
Sorry I just realized I had draft comment on this but I never sent them. Nico seems happy, so feel free to ignore. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.h File base/debug/stack_trace.h (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.h:113: BASE_EXPORT size_t TraceStackFramePointers(const void** out_trace, it's a bit conflicting that you have two APIs publicly exposed (this one and FromFramePointers). Maybe you want only the one up and this one should be an internal function? https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:758: out_trace[depth++] = reinterpret_cast<const void**>(fp)[1]; the frame pointer can be invalid even at this stage, if by accident this function did not emit a frame pointer (because your guessing is wrong). I think you should move some of the basic checks (is this address aligned in a reasonable zone) here, before you dereference fp the first time. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:763: { why this is inside a scope? Is there an if missing ? https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:768: if (next_fp <= fp) break; I think these checks should happen before you dereference fp above no? If the first frame itself is invalid, you will just jump on random with that next_fp = fp[0] https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... File base/debug/stack_trace_unittest.cc (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_unittest.cc:243: TEST_F(StackTraceTest, TraceStackFramePointers) { I wouldn't base this test on comparing unwinding via frame pointers with unwinding via libunwind (or whatever). They will very likely produce different stacks for arbitrary function calls (inlining, & co). I'd much rather do the following: Create a sequence of 5 functions that call into each other. Make sure they get a frame. Test that you see the 5 functions and that's it.
Guys, also please review build files changes - I needed to make sure that base_unittests is built with frame pointers. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.h File base/debug/stack_trace.h (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.h:113: BASE_EXPORT size_t TraceStackFramePointers(const void** out_trace, On 2016/04/18 20:18:07, Primiano Tucci wrote: > it's a bit conflicting that you have two APIs publicly exposed (this one and > FromFramePointers). > Maybe you want only the one up and this one should be an internal function? Actually, the top one is a mistake. Thanks for noticing! https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:751: uintptr_t fp = reinterpret_cast<uintptr_t>(__builtin_frame_address(0)); On 2016/04/18 19:42:05, Nico wrote: > nit: call this sp Done. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:758: out_trace[depth++] = reinterpret_cast<const void**>(fp)[1]; On 2016/04/18 19:42:05, Nico wrote: > you treat fp as void** almost everywhere, consider making it type void** instead > of uintptr_t So we have these casts: reinterpret_cast<uintptr_t>(__builtin_frame_address(0)); reinterpret_cast<const void**>(fp)[1]; reinterpret_cast<const uintptr_t*>(fp)[0]; If we change fp (sp) to be const void*, then we can avoid the first cast, but get issues with checks in exchange. Checks operate on byte differences, so we would need to turn next_fp into const void* too and insert (uintptr_t) casts in every if(). If we change fp to be const void**, then we can avoid second cast, but others two will remain. So no matter how we change fp we will have casts. The current version gets rid of (uintptr_t) casts in checks, so they are cleaner. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:758: out_trace[depth++] = reinterpret_cast<const void**>(fp)[1]; On 2016/04/18 20:18:07, Primiano Tucci wrote: > the frame pointer can be invalid even at this stage, if by accident this > function did not emit a frame pointer (because your guessing is wrong). > I think you should move some of the basic checks (is this address aligned in a > reasonable zone) here, before you dereference fp the first time. Hmm, since I don't have next_fp at that point, I can only check for alignment. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:763: { On 2016/04/18 20:18:08, Primiano Tucci wrote: > why this is inside a scope? Is there an if missing ? Done. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_posix.cc:768: if (next_fp <= fp) break; On 2016/04/18 20:18:08, Primiano Tucci wrote: > I think these checks should happen before you dereference fp above no? > If the first frame itself is invalid, you will just jump on random with that > next_fp = fp[0] But this also dereferences fp, next_fp is fp[1]. https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... File base/debug/stack_trace_unittest.cc (right): https://codereview.chromium.org/1879073002/diff/20001/base/debug/stack_trace_... base/debug/stack_trace_unittest.cc:243: TEST_F(StackTraceTest, TraceStackFramePointers) { On 2016/04/18 20:18:08, Primiano Tucci wrote: > I wouldn't base this test on comparing unwinding via frame pointers with > unwinding via libunwind (or whatever). > They will very likely produce different stacks for arbitrary function calls > (inlining, & co). > I'd much rather do the following: > Create a sequence of 5 functions that call into each other. Make sure they get a > frame. Test that you see the 5 functions and that's it. Done.
hm, it'd be better if we didn't have to use special compiler flags for base_unittests. since call frames are only omitted for leaf functions, can you get the same effect in just your test by letting a few __attribute__((nonline)) functions call each other?
On 2016/04/20 17:11:32, Nico wrote: > hm, it'd be better if we didn't have to use special compiler flags for > base_unittests. since call frames are only omitted for leaf functions, can you > get the same effect in just your test by letting a few __attribute__((nonline)) > functions call each other? Call frames are only omitted for leaf functions? I thought it was other way around - even with no-omit-frame-pointers they can be omitted from leaf functions, see https://llvm.org/bugs/show_bug.cgi?id=9825 Also see try results for patchset 1, where on some platforms I was able to get only 1 frame using frame pointers. I think that was because frame pointers were not enabled. BTW, tests functions are already NOINLINE, see stack_trace_unittest.cc
well, call frames are omitted when the compiler figures it's not needed. aren't they're always needed for non-leaf functions with local variables (or non-tailcall calls, at least on 32-bit)?
On 2016/04/20 17:19:28, Dmitry Skiba wrote: > On 2016/04/20 17:11:32, Nico wrote: > > hm, it'd be better if we didn't have to use special compiler flags for > > base_unittests. since call frames are only omitted for leaf functions, can you > > get the same effect in just your test by letting a few > __attribute__((nonline)) > > functions call each other? > > Call frames are only omitted for leaf functions? I thought it was other way > around - even with no-omit-frame-pointers they can be omitted from leaf > functions, see https://llvm.org/bugs/show_bug.cgi?id=9825 > > Also see try results for patchset 1, where on some platforms I was able to get > only 1 frame using frame pointers. I think that was because frame pointers were > not enabled. > > BTW, tests functions are already NOINLINE, see stack_trace_unittest.cc Yeah I think no-omit-frame-pointers is for all frames, so that the fp can be used as a GP register everywhere. I think that one is no-omit-**leaf**-frame-pointer. Silly question: can we use / rely on __attribute__((optimize("O0")))? Is there a clang equivalent?
On 2016/04/20 17:23:49, Nico wrote: > well, call frames are omitted when the compiler figures it's not needed. aren't > they're always needed for non-leaf functions with local variables (or > non-tailcall calls, at least on 32-bit)? So as I understand the situation is as follows: 1. Usually we don't specify neither no-omit-frame-pointer nor omit-frame-pointer 2. Because of that we get default behavior, which is platform-specific - on i368 frame pointers are ON by default, on x64 they are OFF by default (I think they're also OFF on ARM, but we turn them ON on ARM64 because otherwise we can't unwind). 3. So we need -fno-omit-frame-pointer to make sure that we always get frame pointers, regardless of platform. I screwed up with my last change (-Wno-omit-frame-pointer instead of -f for GYP), so let me try two things: 1. Without buildfile changes 2. With correct flag in GYP
On 2016/04/20 17:23:49, Nico wrote: > well, call frames are omitted when the compiler figures it's not needed. aren't > they're always needed for non-leaf functions with local variables (or > non-tailcall calls, at least on 32-bit)? I think compilers are more aggressive than that. Every time I try to read the gcc doc for fomit-frame-pointer i freak out, lemme try again: From https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Optimize-Options --- The default setting (when not optimizing for size) for 32-bit GNU/Linux x86 and 32-bit Darwin x86 targets is -fomit-frame-pointer. You can configure GCC with the --enable-frame-pointer configure option to change the default. Enabled at levels -O, -O2, -O3, -Os. #### (-Os here feels inconsistent to me with the statement above) --- Which to me sounds like: we all know that frame pointers make the code too slow on x86, so we'll just omit them. Gdb should be able to figure out via DWARF CFI, so if you really really want them tell the compiler to explicitly not omit them. I am pretty confident this works also this way for arm. I never remember what is the deal with the other archs, but my understansing is that "having no expectation" is the right expectation if you don't pass neither -fomit nor -fno-omit. I can tell that on x86 vs aarm64 the default behavior is completely the opposite. See http://pastebin.com/c2BiKQxT on x86 it seems omitted by default, but not on aarm64
The stack_trace_posix.cc code looks good to me. The unittest seems very magical, I defer it to Nico. I had no idea you can even take the address of a label. FWIW it seems that calling __builtin_frame_address(0) in a function forces the compiler to emit frame pointers, even if you explicitly -fomit, see http://pastebin.com/QMi8zWRk So that might solve your unittest problem without having to play with flags of base_unittests
On 2016/04/20 18:53:07, Primiano Tucci wrote: > The stack_trace_posix.cc code looks good to me. > The unittest seems very magical, I defer it to Nico. I had no idea you can even > take the address of a label. > FWIW it seems that calling __builtin_frame_address(0) in a function forces the > compiler to emit frame pointers, even if you explicitly -fomit, see > http://pastebin.com/QMi8zWRk > So that might solve your unittest problem without having to play with flags of > base_unittests I did a bit of digging in LLVM/Clang: 1. When you use __builtin_frame_address(), Intrinsic::frameaddress is generated 2. When Intrinsic::frameaddress is lowered, setFrameAddressIsTaken(true) is set on MachineFrameInfo (e.g. llvm/lib/Target/X86/X86FastISel.cpp:2341) 3. isFrameAddressTaken() is taken into account in hasFP() function (e.g. llvm/lib/Target/X86/X86FrameLowering.cpp:89) 4. hasFP() is then used in emitEpilogue() function (llvm/lib/Target/X86/X86FrameLowering.cpp:1019) So I'm going to use __builtin_frame_address() to force frame pointers in my test functions.
Hurray, test passed on all builders! Nico, are you OK with &&label trick I used?
Description was changed from ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. For now only x86 and x64 POSIX plarforms are supported. BUG=602701 ========== to ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. The function supports x86, x64 and arm (but not thumb) architectures on POSIX platforms. BUG=602701 ==========
Added support for arm. Had to move function to stack_trace.cc because on Android we don't compile stack_trace_posix.cc, we have stack_trace_android.cc instead. Unfortunately it seems that both GCC and LLVM don't generate proper frames in Thumb mode, see https://llvm.org/bugs/show_bug.cgi?id=18505 - the bug is old, but still relevant.
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/1879073002/#ps140001 (title: "Properly exclude Thumb; saner EXPECT_ macro to force frame pointers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1879073002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879073002/140001
Message was sent while issue was closed.
Description was changed from ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. The function supports x86, x64 and arm (but not thumb) architectures on POSIX platforms. BUG=602701 ========== to ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. The function supports x86, x64 and arm (but not thumb) architectures on POSIX platforms. BUG=602701 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
still lgtm, but i feel arm support could've been a separate cl. Primiano says that in practice most functions don't have stack frames in 64-bit x86 (I thought they did, but I'm probably misremembering) -- is that at all useful in practice on 64-bit x86 then?
Message was sent while issue was closed.
On 2016/04/22 16:04:58, Nico wrote: > still lgtm, but i feel arm support could've been a separate cl. > > Primiano says that in practice most functions don't have stack frames in 64-bit > x86 (I thought they did, but I'm probably misremembering) -- is that at all > useful in practice on 64-bit x86 then? Yeah, regular release build probably won't have frame pointers (or at least we don't ask for them, not sure about the defaults, they are complicated). But when used as part of native allocation tracing we require either debug or profiling build, both of them have frame pointers. Thanks for LGTM'ing again, BTW :) I wasn't feeling good about landing it without your approval for the latest changes, but Primiano is OK with them, and bots are gree, and I need to land my main CL today, so I decided to land.
Message was sent while issue was closed.
I thought landing this was a bit aggressive too tbh. But in this case, it happened to work out, I suppose.
Message was sent while issue was closed.
On 2016/04/22 16:10:12, Dmitry Skiba wrote: > > Primiano says that in practice most functions don't have stack frames in > 64-bit > > x86 (I thought they did, but I'm probably misremembering) -- is that at all > > useful in practice on 64-bit x86 then? > > Yeah, regular release build probably won't have frame pointers (or at least we > don't ask for them, not sure about the defaults, they are complicated). But when > used as part of native allocation tracing we require either debug or profiling > build, both of them have frame pointers. Yup the main deal here is targeting profiling=1 and the other dev build cases. > Thanks for LGTM'ing again, BTW :) I wasn't feeling good about landing it without > your approval for the latest changes, but Primiano is OK with them Well, in all honesty my last words were "The unittest seems very magical, I defer it to Nico." Not a big deal here, but I guess I really meant here "I defer it to Nico, *wait for a reply*"
Message was sent while issue was closed.
On 2016/04/22 16:15:53, Primiano Tucci wrote: > On 2016/04/22 16:10:12, Dmitry Skiba wrote: > > > Primiano says that in practice most functions don't have stack frames in > > 64-bit > > > x86 (I thought they did, but I'm probably misremembering) -- is that at all > > > useful in practice on 64-bit x86 then? > > > > Yeah, regular release build probably won't have frame pointers (or at least we > > don't ask for them, not sure about the defaults, they are complicated). But > when > > used as part of native allocation tracing we require either debug or profiling > > build, both of them have frame pointers. > > Yup the main deal here is targeting profiling=1 and the other dev build cases. > > > > Thanks for LGTM'ing again, BTW :) I wasn't feeling good about landing it > without > > your approval for the latest changes, but Primiano is OK with them > Well, in all honesty my last words were "The unittest seems very magical, I > defer it to Nico." > Not a big deal here, but I guess I really meant here "I defer it to Nico, *wait > for a reply*" Oh right :( I wasn't reading it right... OK, time to spend some time in the shame cube...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1907373002/ by apacible@chromium.org. The reason for reverting is: Failure on Linux ChromeOS MSan Tests: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20... See: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20....
Message was sent while issue was closed.
Description was changed from ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. The function supports x86, x64 and arm (but not thumb) architectures on POSIX platforms. BUG=602701 ========== to ========== Add function to trace stack using frame pointers. For memory-infra we need fast stack traces to implement allocation tracing (see https://goo.gl/DFoqfi). StackTrace class uses unwinding and is too slow. This change adds a function that uses frame pointers to walk the stack. The function supports x86, x64 and arm (but not thumb) architectures on POSIX platforms. BUG=602701 Committed: https://crrev.com/ead0969fe721b3a71a18abb9c1203c15e1276aa4 Cr-Commit-Position: refs/heads/master@{#389123} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ead0969fe721b3a71a18abb9c1203c15e1276aa4 Cr-Commit-Position: refs/heads/master@{#389123} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
