Explicitly specify whether to emit frame pointers by default.

Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654
Review-Url: https://codereview.chromium.org/2782063005
Cr-Commit-Position: refs/heads/master@{#462622}
Committed: https://chromium.googlesource.com/chromium/src/+/f7c8a0df4253be5271276dc7f3c1da5ce9b677c1

[erikchen, 2017-03-29 23:02:12]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:03:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-03-29 23:38:11]

erikchen@chromium.org changed reviewers:
+ thakis@chromium.org

[erikchen, 2017-03-29 23:38:11]

thakis: Please review.

[commit-bot: I haz the power, 2017-03-29 23:46:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:46:38]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2017-03-30 01:27:13]

Description was changed from

==========
Explicitly specify the existence [or lack] of frame pointers.

I've removed corresponding logic from base/debug/stack_trace.h, which was
guessing at the existence of frame pointers.

BUG=706116
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, this code is broken and should be removed, but that can be addressed
in a future CL.

BUG=706116, 706654
==========

[erikchen, 2017-03-30 01:27:30]

Description was changed from

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, this code is broken and should be removed, but that can be addressed
in a future CL.

BUG=706116, 706654
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, this code is broken and should be removed, but that can be addressed
in a future CL.

BUG=706116, 706654
==========

[erikchen, 2017-03-30 01:30:37]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 01:31:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:42:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:42:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[erikchen, 2017-03-30 03:35:02]

Description was changed from

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, this code is broken and should be removed, but that can be addressed
in a future CL.

BUG=706116, 706654
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, any code that uses HAVE_TRACE_STACK_FRAME_POINTERS is broken and
should be removed, since it relies on the assumption that all stacks will have
frame pointers, but that information can vary TU to TU.

BUG=706116, 706654
==========

[Primiano Tucci (use gerrit), 2017-03-30 13:15:03]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-03-30 13:15:04]

Some thoughts, +dskiba FYI

> Arguably, any code that uses HAVE_TRACE_STACK_FRAME_POINTERS is broken and
should be removed, since it relies on the assumption that all stacks will have
frame pointers, but that information can vary TU to TU.

I left some comment on the bug. I think that your interpretation of
HAVE_TRACE_STACK_FRAME_POINTERS here doesn't fully match the intent of the code
in base/debug for Android.

https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace.h
File base/debug/stack_trace.h (left):

https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace....
base/debug/stack_trace.h:37: #else  //  defined(__arm__) && !defined(__thumb__)
where did this logic go?
The deal here is that we can use frame pointers only when building in ARM (i.e.
non-thumb mode), which happens IIRC with profiling=1.
Makes sense to move this to build-time, but at very least I'd expect something
conditioned by arm_use_thumb or similar, which I don't see here.

https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1387: if (emit_frame_pointers_by_default) {
(Just some naming bikeshedding)
1. by_default
just a small thought, I personally always found hard to reason on variables
called "do_something_by_default" because I can never tell if the semantic is:
- by default, do something (but in this case, why do we have a variable?)
- do something for all targets, unless a target opts out.

I suppose, this is the case for the latter. If that is the case, why not just
calling this "emit_frame_pointers" ?

2. "emit" vs "omit":
 Unfortunately, people got used to reason on frame pointers in negative logic,
because the compiler flag is called -f(no)-omit-frame-pointer. I'm always up for
using positive logic, but calling this "emit" might confuse a distract reader.

How about calling this just "enabled_frame_pointers"? It would be aligned with
the various enable_XXX that we have in build files.

[erikchen, 2017-03-30 22:12:46]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2017-03-30 22:12:47]

mark: Please review.

thakis is afk for a bit, and you've been active on the crbug.

https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace.h
File base/debug/stack_trace.h (left):

https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace....
base/debug/stack_trace.h:37: #else  //  defined(__arm__) && !defined(__thumb__)
On 2017/03/30 13:15:03, Primiano Tucci wrote:
> where did this logic go?
> The deal here is that we can use frame pointers only when building in ARM
(i.e.
> non-thumb mode), which happens IIRC with profiling=1.
> Makes sense to move this to build-time, but at very least I'd expect something
> conditioned by arm_use_thumb or similar, which I don't see here.

The !thumb logic was added here:
https://bugs.chromium.org/p/chromium/issues/detail?id=602701#c5

Either:
1) We can't emit frame_pointers on thumb. I would hope that clang throws an
error when we try to force frame pointers anyways. In this case, we should
update the logic in compiler.gni to accurately reflect this.

2) For some reason, stack walking with frame pointers doesn't work on thumb. In
this case, we should update the stack-walking logic /conditionals to indicate
this. It has nothing to do with whether stacks have frame pointers.

3) Everything works fine on thumb, and this CL doesn't need to change.

https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1387: if (emit_frame_pointers_by_default) {
On 2017/03/30 13:15:03, Primiano Tucci wrote:
> (Just some naming bikeshedding)
> 1. by_default
> just a small thought, I personally always found hard to reason on variables
> called "do_something_by_default" because I can never tell if the semantic is:
> - by default, do something (but in this case, why do we have a variable?)
> - do something for all targets, unless a target opts out.
> 
> I suppose, this is the case for the latter. If that is the case, why not just
> calling this "emit_frame_pointers" ?
> 
> 2. "emit" vs "omit":
>  Unfortunately, people got used to reason on frame pointers in negative logic,
> because the compiler flag is called -f(no)-omit-frame-pointer. I'm always up
for
> using positive logic, but calling this "emit" might confuse a distract reader.
> 
> How about calling this just "enabled_frame_pointers"? It would be aligned with
> the various enable_XXX that we have in build files.

Done.

[Primiano Tucci (use gerrit), 2017-03-30 22:27:18]

On 2017/03/30 22:12:47, erikchen wrote:
> mark: Please review.
> 
> thakis is afk for a bit, and you've been active on the crbug.
> 
> https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace.h
> File base/debug/stack_trace.h (left):
> 
>
https://codereview.chromium.org/2782063005/diff/40001/base/debug/stack_trace....
> base/debug/stack_trace.h:37: #else  //  defined(__arm__) &&
!defined(__thumb__)
> On 2017/03/30 13:15:03, Primiano Tucci wrote:
> > where did this logic go?
> > The deal here is that we can use frame pointers only when building in ARM
> (i.e.
> > non-thumb mode), which happens IIRC with profiling=1.
> > Makes sense to move this to build-time, but at very least I'd expect
something
> > conditioned by arm_use_thumb or similar, which I don't see here.
> 
> The !thumb logic was added here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=602701#c5
> 
> Either:
> 1) We can't emit frame_pointers on thumb. I would hope that clang throws an
> error when we try to force frame pointers anyways. In this case, we should
> update the logic in compiler.gni to accurately reflect this.
> 
> 2) For some reason, stack walking with frame pointers doesn't work on thumb.
In
> this case, we should update the stack-walking logic /conditionals to indicate
> this. It has nothing to do with whether stacks have frame pointers.

This. 
I'm thumb gcc and clang don't agree on the stack frame layout. On top of this,
the stack frame layout that gcc choose doesn't allow to make any use of the
frame pointer, as it's location in the stack is not fixed w.r.t. The return
address. 
https://bugs.llvm.org/show_bug.cgi?id=18505

But by thinking more with your inputs o think we don't really need a macro and
can just add a function book IsStackUnwindingPossible() {
#if android and thumb and omit unwind tables 
Return false 
# else if android and arm and have_frane_pointers
Return true 
... 

My only doubt is: how do you express that "have_frame_pointers" there? You need
it to be passed from the build system (although no need to be exported anywhere
else than I'm the file that defines this function) 
 
> 
> 3) Everything works fine on thumb, and this CL doesn't need to change.
> 
>
https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
> File build/config/compiler/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2782063005/diff/40001/build/config/compiler/B...
> build/config/compiler/BUILD.gn:1387: if (emit_frame_pointers_by_default) {
> On 2017/03/30 13:15:03, Primiano Tucci wrote:
> > (Just some naming bikeshedding)
> > 1. by_default
> > just a small thought, I personally always found hard to reason on variables
> > called "do_something_by_default" because I can never tell if the semantic
is:
> > - by default, do something (but in this case, why do we have a variable?)
> > - do something for all targets, unless a target opts out.
> > 
> > I suppose, this is the case for the latter. If that is the case, why not
just
> > calling this "emit_frame_pointers" ?
> > 
> > 2. "emit" vs "omit":
> >  Unfortunately, people got used to reason on frame pointers in negative
logic,
> > because the compiler flag is called -f(no)-omit-frame-pointer. I'm always up
> for
> > using positive logic, but calling this "emit" might confuse a distract
reader.
> > 
> > How about calling this just "enabled_frame_pointers"? It would be aligned
with
> > the various enable_XXX that we have in build files.
> 
> Done.

[erikchen, 2017-03-30 23:07:57]

Description was changed from

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

Arguably, any code that uses HAVE_TRACE_STACK_FRAME_POINTERS is broken and
should be removed, since it relies on the assumption that all stacks will have
frame pointers, but that information can vary TU to TU.

BUG=706116, 706654
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654
==========

[erikchen, 2017-03-30 23:10:07]

> This. 
> I'm thumb gcc and clang don't agree on the stack frame layout. On top of this,
> the stack frame layout that gcc choose doesn't allow to make any use of the
> frame pointer, as it's location in the stack is not fixed w.r.t. The return
> address. 
> https://bugs.llvm.org/show_bug.cgi?id=18505
> 
> But by thinking more with your inputs o think we don't really need a macro and
> can just add a function book IsStackUnwindingPossible() {
> #if android and thumb and omit unwind tables 
> Return false 
> # else if android and arm and have_frane_pointers
> Return true 
> ... 
> 
> My only doubt is: how do you express that "have_frame_pointers" there? You
need
> it to be passed from the build system (although no need to be exported
anywhere
> else than I'm the file that defines this function) 

The build system passes a preprocessor definition: ENABLED_FRAME_POINTERS, which
describes whether most stacks will have frame pointers. 
There's some minimal logic in base/debug/stack_trace.h that sets
CAN_UNWIND_WITH_FRAME_POINTERS, which depends on ENABLED_FRAME_POINTERS and
thumbiness of architecture.

[erikchen, 2017-03-30 23:27:06]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:28:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:36:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 23:36:53]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mark Mentovai, 2017-03-30 23:43:30]

I’m not really the GN guy you may think I am.

https://codereview.chromium.org/2782063005/diff/120001/base/android/jni_andro...
File base/android/jni_android.h (right):

https://codereview.chromium.org/2782063005/diff/120001/base/android/jni_andro...
base/android/jni_android.h:20: #if CAN_UNWIND_WITH_FRAME_POINTERS
Are you still planning on removing this from headers?

https://codereview.chromium.org/2782063005/diff/120001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2782063005/diff/120001/base/debug/stack_trace...
base/debug/stack_trace.h:29: #if ENABLED_FRAME_POINTERS && !(defined(__arm__) &&
defined(__thumb__))
Don’t you need to #include something to get ENABLED_FRAME_POINTERS set properly?

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
build/config/compiler/BUILD.gn:1390: cflags = [ "-fomit-frame-pointer" ]
I think we’d never want this in a debug-ish configuration, which sounds like
it’s what common_optimize_on_cflags gives you for Windows. Why should
non-Windows behave differently?

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
build/config/compiler/compiler.gni:76: using_sanitizer || enable_profiling ||
is_debug || current_cpu == "arm64"
Maybe this takes care of it? I’m not really a GN guy, I don’t know. I’ll defer
to Nico here (he was around today), or if you can’t get a response to him, I’ll
delegate this to someone else who knows GN better. I’m not an OWNER in here
anyway.

[erikchen, 2017-03-31 01:51:37]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 01:52:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 04:21:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 04:21:18]

Dry run: This issue passed the CQ dry run.

[DmitrySkiba, 2017-03-31 09:58:24]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-03-31 09:58:25]

I think this CL does too much, see
https://bugs.chromium.org/p/chromium/issues/detail?id=706654#c10

I think this CL should only do GN-related changes, and not touch
HAVE_TRACE_STACK_FRAME_POINTERS, because HAVE_TRACE_STACK_FRAME_POINTERS answers
the question "does TraceStackFramePointers support unwinding using frame
pointers on the current platform" (i.e. the macro is literally
HAVE_TraceStackFramePointers), and is doesn't care about whether fps are enabled
for the current build or not.

And for GN changes, I don't think we need to go through debuggin_flags, instead
we should define the macro explicitly via defines (or even cflags):
1. First, it just feels wrong (debugging_flags?)
2. It requires including debugging_flags.h
3. Builds including debugging_flags.h in headers can be flaky (see wez@ blues in
crrev.com/2757123002)

Bonus points: define the macro on some platforms where -fno-omit-frame-pointers
is the default (like macOS?).

I also want to bikeshed on the flag name: I think it should be just
HAVE_FRAME_POINTERS.

[erikchen, 2017-04-01 01:52:57]

On 2017/03/31 09:58:25, DmitrySkiba wrote:
> I think this CL does too much, see
> https://bugs.chromium.org/p/chromium/issues/detail?id=706654#c10
Sorry, but I disagree.

> 
> I think this CL should only do GN-related changes, and not touch
> HAVE_TRACE_STACK_FRAME_POINTERS, because HAVE_TRACE_STACK_FRAME_POINTERS
answers
> the question "does TraceStackFramePointers support unwinding using frame
> pointers on the current platform" (i.e. the macro is literally
> HAVE_TraceStackFramePointers), and is doesn't care about whether fps are
enabled
> for the current build or not.
I now understand why you originally defined this macro. I think this is no
longer appropriate for the current code, as it duplicates logic that already
exists in GN.

> 
> And for GN changes, I don't think we need to go through debuggin_flags,
instead
> we should define the macro explicitly via defines (or even cflags):
> 1. First, it just feels wrong (debugging_flags?)
I was just copying ENABLE_PROFILING.

> 2. It requires including debugging_flags.h
Yes. This is good - this prevents leaking configs to TUs that don't care about
it.

> 3. Builds including debugging_flags.h in headers can be flaky (see wez@ blues
in
> crrev.com/2757123002)
wez@ points out that this is because the header generator rule is incorrect:
https://bugs.chromium.org/p/chromium/issues/detail?id=706654#c11

> 
> Bonus points: define the macro on some platforms where
-fno-omit-frame-pointers
> is the default (like macOS?).
This requires logic duplication. This CL already does the right thing.

> 
> I also want to bikeshed on the flag name: I think it should be just
> HAVE_FRAME_POINTERS.
Sorry, not taking more bikeshedding on names.

[erikchen, 2017-04-03 06:25:31]

Description was changed from

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL makes it so that Windows is actually affected by
config("default_stack_frames"), previously the logic lived elsewhere in the
file, and was not configurable.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654
==========

[erikchen, 2017-04-03 06:33:39]

https://codereview.chromium.org/2782063005/diff/120001/base/android/jni_andro...
File base/android/jni_android.h (right):

https://codereview.chromium.org/2782063005/diff/120001/base/android/jni_andro...
base/android/jni_android.h:20: #if CAN_UNWIND_WITH_FRAME_POINTERS
On 2017/03/30 23:43:29, Mark Mentovai wrote:
> Are you still planning on removing this from headers?

No. Issue 706654 is the tracking bug to either fix or remove this macro.

https://codereview.chromium.org/2782063005/diff/120001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2782063005/diff/120001/base/debug/stack_trace...
base/debug/stack_trace.h:29: #if ENABLED_FRAME_POINTERS && !(defined(__arm__) &&
defined(__thumb__))
On 2017/03/30 23:43:29, Mark Mentovai wrote:
> Don’t you need to #include something to get ENABLED_FRAME_POINTERS set
properly?

yup

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2782063005/diff/120001/build/config/compiler/...
build/config/compiler/BUILD.gn:1390: cflags = [ "-fomit-frame-pointer" ]
On 2017/03/30 23:43:29, Mark Mentovai wrote:
> I think we’d never want this in a debug-ish configuration, which sounds like
> it’s what common_optimize_on_cflags gives you for Windows. Why should
> non-Windows behave differently?

I'm just maintaining the previous behavior.

[erikchen, 2017-04-03 06:33:44]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 06:33:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 08:10:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 08:10:28]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-04-03 17:46:51]

erikchen@chromium.org changed reviewers:
+ brettw@chromium.org

[erikchen, 2017-04-03 17:46:52]

brettw: Please review.

[brettw, 2017-04-03 21:17:43]

lgtm

https://codereview.chromium.org/2782063005/diff/180001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2782063005/diff/180001/base/debug/stack_trace...
base/debug/stack_trace.h:30: #if ENABLED_FRAME_POINTERS && !(defined(__arm__) &&
defined(__thumb__))
This isn't how you use the buildflag header. So you will want to do
BUILDFLAG(...) and also double-check that this is getting set the way you want.

https://codereview.chromium.org/2782063005/diff/180001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2782063005/diff/180001/build/config/compiler/...
build/config/compiler/BUILD.gn:1396: # "optimize" confnig. Similarly, there is
no way to propagate state from this
confnig spelling

[erikchen, 2017-04-03 21:40:40]

I spoke with dskiba@ offline. He said that he wants there to be only a single
macro [e.g. remove the conditional from base/debug/stack_trace.h] and move it
into the compiler.gni file.I'm fine with that.

[erikchen, 2017-04-03 22:42:05]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:42:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:50:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 22:50:09]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[erikchen, 2017-04-04 18:24:53]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-04 18:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2782063005/#ps260001
(title: "compile fixes.")

[commit-bot: I haz the power, 2017-04-04 18:25:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 18:33:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:33:46]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[erikchen, 2017-04-04 18:41:10]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-04 18:41:10]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2782063005/#ps280001
(title: "missing gn import.")

[commit-bot: I haz the power, 2017-04-04 18:42:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 18:55:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:55:21]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[erikchen, 2017-04-04 23:11:03]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-04 23:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2782063005/#ps300001
(title: "arm64 -> arm")

[commit-bot: I haz the power, 2017-04-04 23:12:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 00:40:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 00:40:55]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[erikchen, 2017-04-05 19:16:31]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-05 19:16:31]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2782063005/#ps320001
(title: "Add dependency from base_paths onto debugging_flags.")

[commit-bot: I haz the power, 2017-04-05 19:17:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-04-05 20:33:12]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-04-05 23:29:53]

Patchset #17 (id:320001) has been deleted

[erikchen, 2017-04-05 23:29:59]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-04-05 23:30:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 03:50:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 03:50:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2017-04-06 17:32:22]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-04-06 17:34:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 21:15:31]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491499942985420,
"parent_rev": "48296219d472221d30d3a8cc4d03317ee2dd2851", "commit_rev":
"f7c8a0df4253be5271276dc7f3c1da5ce9b677c1"}

[commit-bot: I haz the power, 2017-04-06 21:16:55]

Description was changed from

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654
==========

to

==========
Explicitly specify whether to emit frame pointers by default.

All platforms now specify whether to emit frame pointers by default, rather than
relying on default compiler options.

This CL moves the logic from config("default_stack_frames") into compiler.gni.
The former is actually the right place for the logic to live, but there exists
code that relies on whether a frame pointer is emitted by default. Right now,
that logic is being duplicated/guessed by the code in question. This CL at least
unifies the logic in a single location.

There current exists code that uses a preprocessor definition
HAVE_TRACE_STACK_FRAME_POINTERS. Despite the name, the code really wants to know
if most stacks can be unwound using stack pointers. I've renamed it to
CAN_UNWIND_WITH_FRAME_POINTERS. Arguably, any code that uses
CAN_UNWIND_WITH_FRAME_POINTERS is broken and should be removed, since it relies
on the assumption that all stacks will either have or not have frame pointers,
but that can vary TU by TU.

BUG=706116, 706654

Review-Url: https://codereview.chromium.org/2782063005
Cr-Commit-Position: refs/heads/master@{#462622}
Committed:
https://chromium.googlesource.com/chromium/src/+/f7c8a0df4253be5271276dc7f3c1...
==========

[commit-bot: I haz the power, 2017-04-06 21:16:57]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/f7c8a0df4253be5271276dc7f3c1...