Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2266073002:
Explicitly ask for stack frame pointers on Debug posix builds. (Closed)
DescriptionExplicitly ask for stack frame pointers on Debug posix builds.
GCC / LLVM can omit stack frames at any optimization level. We use -Os
for Android Debug, and -O3 for targets like v8. This can cause the runtime
stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash.
This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well
as fixing consistency for the setting in profiling and stanitizer builds.
The -f*omit-frame-pointer flag settings are moved to
config("default_stack_frames"), to enable build targets to disable and
override the default settings.
R=brettw@chromium.org
R=dpranke@chromium.org
BUG=636489
Committed: https://crrev.com/ecdecf5e9fec3c61496f3cedd78c0ba146eb5b91
Cr-Commit-Position: refs/heads/master@{#415102}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move stack frame flag to it's own config. #
Total comments: 4
Patch Set 3 : Keep -fomit-frame-pointer for non-debug Android builds. #
Total comments: 4
Patch Set 4 : Rebased after staging changes to ffmpeg. #Messages
Total messages: 44 (18 generated)
The CQ bit was checked by vmiura@chromium.org to run a CQ dry run
ptal
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ========== to ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ==========
https://codereview.chromium.org/2266073002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:238: # crashes when unwinding stack frames (crbug.com/636489). Do we have any idea what the perf impact of -fno-omit-frame-pointer is? I've been thinking that maybe it should just be on unconditionally in non-official builds. Alternatively, we might want to make it be a configurable argument; the goal of either choice would partially be to make sure stack traces are as debuggable/useful as possible.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2266073002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:238: # crashes when unwinding stack frames (crbug.com/636489). On 2016/08/22 20:03:48, Dirk Pranke wrote: > Do we have any idea what the perf impact of -fno-omit-frame-pointer is? I've > been thinking that maybe it should just be on unconditionally in non-official > builds. > > Alternatively, we might want to make it be a configurable argument; the goal of > either choice would partially be to make sure stack traces are as > debuggable/useful as possible. I don't have data but I expect the impact is fairly small. The 'enable_profiling' flag partly overlaps with that. Perhaps a follow up?
Seems fine. lgtm.
kbr@chromium.org changed reviewers: + kbr@chromium.org
Thanks Victor for tracking this down. Do you think the build failures are related? Maybe because this forced a full rebuild on some platforms, and that times out?
On 2016/08/22 21:04:20, Ken Russell wrote: > Thanks Victor for tracking this down. Do you think the build failures are > related? Maybe because this forced a full rebuild on some platforms, and that > times out? Yesh, odd. Taking two hours to build ~40,000 targets. Isn't Goma used on these?
All I can tell so far is that some objects including parts of v8 take several minutes to compile. Goma is running, but parallelism is limited across some targets. The build time seems to drop once Goma has cached the updated objects, so I'm going to retry.
The CQ bit was checked by vmiura@chromium.org
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.
Description was changed from ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ========== to ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ========== to ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 Committed: https://crrev.com/4e69ee6824fc94c59762b5f05f9f340fb4466d7f Cr-Commit-Position: refs/heads/master@{#413628} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4e69ee6824fc94c59762b5f05f9f340fb4466d7f Cr-Commit-Position: refs/heads/master@{#413628}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2269063002/ by johnme@chromium.org. The reason for reverting is: This broke all Android x86/x64 debug bots, for example: https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builde... https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde... It broke because ffmpeg expects to be compiled with -fomit-frame-pointer so that files like third_party/ffmpeg/libavcodec/x86/mpegaudiodsp.c:86 can use an extra register; globally applying -fno-omit-frame-pointer appears to have caused it to run out of registers. If you look at third_party/ffmpeg/ffmpeg.gyp:260, you'll see it removes this flag if it has been set globally: 'cflags!': [ '-fno-omit-frame-pointer', ], But the GN equivalent third_party/ffmpeg/BUILD.gn can't do this, because it's an error in GN to remove a flag that hasn't been set. A clean solution is probably to create a new config in build/config/compiler/BUILD.gn providing the default no-omit-frame-pointer cflag, that is always included, then third_party/ffmpeg/BUILD.gn can unconditionally remove that config and set its own omit-frame-pointer cflag..
Message was sent while issue was closed.
PTAL. Does adding a 'default_stack_frames' config which ffmpeg can remove look like the right direction? If yes, I'll need to stage the change across multiple patches to chromium & third_party/ffmpeg. An alternative would be to make an 'optimize_max_no_stack_frames' config and switch ffmpeg to use that.
Message was sent while issue was closed.
https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ "-fomit-frame-pointer" ] This was lost. It's not clear to me that the intent is that we now add stack frames on Android release builds.
Message was sent while issue was closed.
https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ "-fomit-frame-pointer" ] On 2016/08/23 22:26:43, brettw (ping on IM after 24h) wrote: > This was lost. It's not clear to me that the intent is that we now add stack > frames on Android release builds. You're right. I removed this here as well as the -Os build on Android below, because -fomit-frame-pointer should be implied by an -O* flag. https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Optimize-Options.html "-O also turns on -fomit-frame-pointer on machines where doing so does not interfere with debugging."
Message was sent while issue was closed.
Generally this pattern of having a default config that is removed by the sub target feels awkward. Are there any good alternatives in GN? Could ffmpeg override a variable that a config uses instead? https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ "-fomit-frame-pointer" ] Thinking some more, we probably want to force -fomit-frame-pointer on Release builds even if it may interfere with debugging. I'll fix this.
Message was sent while issue was closed.
Dropping explicit -fomit-frame-pointer from optimized Android builds increased APK size by 275KB. Added it back, and confirmed the APK size is back as before. https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ "-fomit-frame-pointer" ] On 2016/08/24 00:14:45, vmiura OOO back 8-15 wrote: > Thinking some more, we probably want to force -fomit-frame-pointer on Release > builds even if it may interfere with debugging. I'll fix this. Done.
Message was sent while issue was closed.
Description was changed from ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 Committed: https://crrev.com/4e69ee6824fc94c59762b5f05f9f340fb4466d7f Cr-Commit-Position: refs/heads/master@{#413628} ========== to ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 Committed: https://crrev.com/4e69ee6824fc94c59762b5f05f9f340fb4466d7f Cr-Commit-Position: refs/heads/master@{#413628} ==========
On 2016/08/24 01:06:50, vmiura OOO back 8-15 wrote: > Dropping explicit -fomit-frame-pointer from optimized Android builds increased > APK size by 275KB. Added it back, and confirmed the APK size is back as before. > > https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (left): > > https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... > build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ > "-fomit-frame-pointer" ] > On 2016/08/24 00:14:45, vmiura OOO back 8-15 wrote: > > Thinking some more, we probably want to force -fomit-frame-pointer on Release > > builds even if it may interfere with debugging. I'll fix this. > > Done. Why do we want this always on in Release builds? As to your earlier comments, having to split flags into separate configs and explicitly and enable/disable them per target is the only way to do this. And, if we end up needing a config per flag we're probably in a bad place, but we're not there yet.
https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1293: if (using_sanitizer || enable_profiling || is_debug || This is still different from the old code on Android. Previously on Android no_optimize would still omit frame pointers unless !android_full_debug was set (which is what you set when you really want to debug).
On 2016/08/24 18:21:13, Dirk Pranke wrote: > On 2016/08/24 01:06:50, vmiura OOO back 8-15 wrote: > > Dropping explicit -fomit-frame-pointer from optimized Android builds increased > > APK size by 275KB. Added it back, and confirmed the APK size is back as > before. > > > > > https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... > > File build/config/compiler/BUILD.gn (left): > > > > > https://codereview.chromium.org/2266073002/diff/20001/build/config/compiler/B... > > build/config/compiler/BUILD.gn:1245: common_optimize_on_cflags += [ > > "-fomit-frame-pointer" ] > > On 2016/08/24 00:14:45, vmiura OOO back 8-15 wrote: > > > Thinking some more, we probably want to force -fomit-frame-pointer on > Release > > > builds even if it may interfere with debugging. I'll fix this. > > > > Done. > > Why do we want this always on in Release builds? It was on before, and removing it increases APK size by 275KB. We could keep it only for official builds, but I don't see harm in keeping it on for Release right now.
https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1293: if (using_sanitizer || enable_profiling || is_debug || On 2016/08/24 18:27:02, brettw (ping on IM after 24h) wrote: > This is still different from the old code on Android. > > Previously on Android no_optimize would still omit frame pointers unless > !android_full_debug was set (which is what you set when you really want to > debug). I may be missing a nuance, but I'm intentionally making this change from the old behavior because omitting frames on Debug can cause flaky crashes in tcmalloc. We run standard Debug builds on bots and I don't think we want to change them to android_full_debug.
https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1301: cflags = [ "-fomit-frame-pointer" ] I mean this line. It used to not set this on the Android debug case when android_full_debug was set.
https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2266073002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1301: cflags = [ "-fomit-frame-pointer" ] On 2016/08/24 19:30:02, brettw (ping on IM after 24h) wrote: > I mean this line. It used to not set this on the Android debug case when > android_full_debug was set. Thanks, yeah I think that 'android_full_debug' is only used together with 'is_debug' which triggers config("no_optimize"), so it would use the -fno-omit-frame-pointer flag.
I see, I misunderstood. LGTM
Description was changed from ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 Committed: https://crrev.com/4e69ee6824fc94c59762b5f05f9f340fb4466d7f Cr-Commit-Position: refs/heads/master@{#413628} ========== to ========== Explicitly ask for stack frame pointers on Debug posix builds. GCC / LLVM can omit stack frames at any optimization level. We use -Os for Android Debug, and -O3 for targets like v8. This can cause the runtime stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash. This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well as fixing consistency for the setting in profiling and stanitizer builds. The -f*omit-frame-pointer flag settings are moved to config("default_stack_frames"), to enable build targets to disable and override the default settings. R=brettw@chromium.org R=dpranke@chromium.org BUG=636489 ==========
The CQ bit was checked by vmiura@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by vmiura@chromium.org
The CQ bit was checked by vmiura@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2266073002/#ps60001 (title: "Rebased after staging changes to ffmpeg.")
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.
Description was changed from
==========
Explicitly ask for stack frame pointers on Debug posix builds.
GCC / LLVM can omit stack frames at any optimization level. We use -Os
for Android Debug, and -O3 for targets like v8. This can cause the runtime
stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash.
This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well
as fixing consistency for the setting in profiling and stanitizer builds.
The -f*omit-frame-pointer flag settings are moved to
config("default_stack_frames"), to enable build targets to disable and
override the default settings.
R=brettw@chromium.org
R=dpranke@chromium.org
BUG=636489
==========
to
==========
Explicitly ask for stack frame pointers on Debug posix builds.
GCC / LLVM can omit stack frames at any optimization level. We use -Os
for Android Debug, and -O3 for targets like v8. This can cause the runtime
stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash.
This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well
as fixing consistency for the setting in profiling and stanitizer builds.
The -f*omit-frame-pointer flag settings are moved to
config("default_stack_frames"), to enable build targets to disable and
override the default settings.
R=brettw@chromium.org
R=dpranke@chromium.org
BUG=636489
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Explicitly ask for stack frame pointers on Debug posix builds.
GCC / LLVM can omit stack frames at any optimization level. We use -Os
for Android Debug, and -O3 for targets like v8. This can cause the runtime
stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash.
This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well
as fixing consistency for the setting in profiling and stanitizer builds.
The -f*omit-frame-pointer flag settings are moved to
config("default_stack_frames"), to enable build targets to disable and
override the default settings.
R=brettw@chromium.org
R=dpranke@chromium.org
BUG=636489
==========
to
==========
Explicitly ask for stack frame pointers on Debug posix builds.
GCC / LLVM can omit stack frames at any optimization level. We use -Os
for Android Debug, and -O3 for targets like v8. This can cause the runtime
stack unwinding debug feature of 'tcmalloc' to mis-unwind and crash.
This CL adds "-fno-omit-frame-pointer" by default to Debug builds, as well
as fixing consistency for the setting in profiling and stanitizer builds.
The -f*omit-frame-pointer flag settings are moved to
config("default_stack_frames"), to enable build targets to disable and
override the default settings.
R=brettw@chromium.org
R=dpranke@chromium.org
BUG=636489
Committed: https://crrev.com/ecdecf5e9fec3c61496f3cedd78c0ba146eb5b91
Cr-Commit-Position: refs/heads/master@{#415102}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ecdecf5e9fec3c61496f3cedd78c0ba146eb5b91 Cr-Commit-Position: refs/heads/master@{#415102} |
