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}

base/BUILD.gn

@@ -1626,6 +1626,7 @@ buildflag_header("debugging_flags") {
   flags = [
     "ENABLE_PROFILING=$enable_profiling",
     "ENABLE_MEMORY_TASK_PROFILER=$enable_memory_task_profiler",
+    "CAN_UNWIND_WITH_FRAME_POINTERS=$can_unwind_with_frame_pointers",
   ]
 }
 

base/android/jni_android.cc

@@ -29,7 +29,7 @@ base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject>>::Leaky
     g_class_loader = LAZY_INSTANCE_INITIALIZER;
 jmethodID g_class_loader_load_class_method_id = 0;
 
-#if BUILDFLAG(ENABLE_PROFILING) && HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 base::LazyInstance<base::ThreadLocalPointer<void>>::Leaky
     g_stack_frame_pointer = LAZY_INSTANCE_INITIALIZER;
 #endif
@@ -289,7 +289,7 @@ std::string GetJavaExceptionInfo(JNIEnv* env, jthrowable java_throwable) {
   return ConvertJavaStringToUTF8(exception_string);
 }
 
-#if BUILDFLAG(ENABLE_PROFILING) && HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 JNIStackFrameSaver::JNIStackFrameSaver(void* current_fp) {
   previous_fp_ = g_stack_frame_pointer.Pointer()->Get();
@@ -304,7 +304,7 @@ void* JNIStackFrameSaver::SavedFrame() {
   return g_stack_frame_pointer.Pointer()->Get();
 }
 
-#endif  // ENABLE_PROFILING && HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 }  // namespace android
 }  // namespace base

base/android/jni_android.h

@@ -14,10 +14,11 @@
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
+#include "base/debug/debugging_flags.h"
 #include "base/debug/stack_trace.h"
 #include "base/macros.h"
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 // When profiling is enabled (enable_profiling=true) this macro is added to
 // all generated JNI stubs so that it becomes the last thing that runs before
@@ -46,7 +47,7 @@
 #define JNI_SAVE_FRAME_POINTER
 #define JNI_LINK_SAVED_FRAME_POINTER
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 namespace base {
 namespace android {
@@ -166,7 +167,7 @@ BASE_EXPORT void CheckException(JNIEnv* env);
 BASE_EXPORT std::string GetJavaExceptionInfo(JNIEnv* env,
                                              jthrowable java_throwable);
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 // Saves caller's PC and stack frame in a thread-local variable.
 // Implemented only when profiling is enabled (enable_profiling=true).
@@ -182,7 +183,7 @@ class BASE_EXPORT JNIStackFrameSaver {
   DISALLOW_COPY_AND_ASSIGN(JNIStackFrameSaver);
 };
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 }  // namespace android
 }  // namespace base

base/debug/stack_trace.cc

@@ -12,7 +12,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 #include <pthread.h>
@@ -28,14 +28,14 @@
 extern "C" void* __libc_stack_end;
 #endif
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 namespace base {
 namespace debug {
 
 namespace {
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS && !defined(OS_WIN)
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(OS_WIN)
 
 #if defined(__arm__) && defined(__GNUC__) && !defined(__clang__)
 // GCC and LLVM generate slightly different frames on ARM, see
@@ -142,11 +142,11 @@ void* LinkStackFrames(void* fpp, void* parent_fp) {
   return prev_parent_fp;
 }
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS && !defined(OS_WIN)
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(OS_WIN)
 
 }  // namespace
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 uintptr_t GetStackEnd() {
 #if defined(OS_ANDROID)
   // Bionic reads proc/maps on every call to pthread_getattr_np() when called
@@ -194,7 +194,7 @@ uintptr_t GetStackEnd() {
   // Don't know how to get end of the stack.
   return 0;
 }
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 StackTrace::StackTrace() : StackTrace(arraysize(trace_)) {}
 
@@ -220,7 +220,7 @@ std::string StackTrace::ToString() const {
   return stream.str();
 }
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 size_t TraceStackFramePointers(const void** out_trace,
                                size_t max_depth,
@@ -286,7 +286,7 @@ ScopedStackFrameLinker::~ScopedStackFrameLinker() {
 }
 #endif  // !defined(OS_WIN)
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 }  // namespace debug
 }  // namespace base

base/debug/stack_trace.h

@@ -11,6 +11,7 @@
 #include <string>
 
 #include "base/base_export.h"
+#include "base/debug/debugging_flags.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 
@@ -23,24 +24,6 @@ struct _EXCEPTION_POINTERS;
 struct _CONTEXT;
 #endif
 
-// TODO(699863): Clean up HAVE_TRACE_STACK_FRAME_POINTERS.
-#if defined(OS_POSIX)
-
-#if defined(__i386__) || defined(__x86_64__)
-#define HAVE_TRACE_STACK_FRAME_POINTERS 1
-#elif defined(__arm__) && !defined(__thumb__)
-#define HAVE_TRACE_STACK_FRAME_POINTERS 1
-#else  // defined(__arm__) && !defined(__thumb__)
-#define HAVE_TRACE_STACK_FRAME_POINTERS 0
-#endif  // defined(__arm__) && !defined(__thumb__)
-
-#elif defined(OS_WIN)
-#define HAVE_TRACE_STACK_FRAME_POINTERS 1
-
-#else  // defined(OS_WIN)
-#define HAVE_TRACE_STACK_FRAME_POINTERS 0
-#endif  // defined(OS_WIN)
-
 namespace base {
 namespace debug {
 
@@ -56,7 +39,7 @@ namespace debug {
 BASE_EXPORT bool EnableInProcessStackDumping();
 
 // Returns end of the stack, or 0 if we couldn't get it.
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 BASE_EXPORT uintptr_t GetStackEnd();
 #endif
 
@@ -119,7 +102,7 @@ class BASE_EXPORT StackTrace {
   size_t count_;
 };
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 // Traces the stack by using frame pointers. This function is faster but less
 // reliable than StackTrace. It should work for debug and profiling builds,
 // but not for release builds (although there are some exceptions).
@@ -184,7 +167,7 @@ class BASE_EXPORT ScopedStackFrameLinker {
 };
 #endif  // !defined(OS_WIN)
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
 namespace internal {
 

base/debug/stack_trace_unittest.cc

@@ -8,6 +8,7 @@
 #include <sstream>
 #include <string>
 
+#include "base/debug/debugging_flags.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/process/kill.h"
@@ -254,10 +255,11 @@ TEST_F(StackTraceTest, itoa_r) {
 }
 #endif  // defined(OS_POSIX) && !defined(OS_ANDROID)
 
-#if HAVE_TRACE_STACK_FRAME_POINTERS && !defined(OS_WIN)
 // Windows x64 binaries cannot be built with frame pointer, and MSVC doesn't
 // provide intrinsics to query the frame pointer even for the x86 build, nor
 // does it allow us to take the address of labels, so skip these under Windows.
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(OS_WIN)
+
 template <size_t Depth>
 void NOINLINE ExpectStackFramePointers(const void** frames,
                                        size_t max_depth) {
@@ -315,7 +317,7 @@ TEST_F(StackTraceTest, MAYBE_StackEnd) {
   EXPECT_NE(0u, GetStackEnd());
 }
 
-#endif  // HAVE_TRACE_STACK_FRAME_POINTERS && !defined(OS_WIN)
+#endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(OS_WIN)
 
 }  // namespace debug
 }  // namespace base

base/trace_event/heap_profiler_allocation_context_tracker.cc

@@ -8,6 +8,7 @@
 #include <iterator>
 
 #include "base/atomicops.h"
+#include "base/debug/debugging_flags.h"
 #include "base/debug/leak_annotations.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_local_storage.h"
@@ -206,7 +207,7 @@ bool AllocationContextTracker::GetContextSnapshot(AllocationContext* ctx) {
         const void* frames[128];
         static_assert(arraysize(frames) >= Backtrace::kMaxFrameCount,
                       "not requesting enough frames to fill Backtrace");
-#if HAVE_TRACE_STACK_FRAME_POINTERS && !defined(OS_NACL)
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(OS_NACL)
         size_t frame_count = debug::TraceStackFramePointers(
             frames,
             arraysize(frames),

base/trace_event/memory_dump_manager.cc

@@ -195,8 +195,7 @@ void MemoryDumpManager::EnableHeapProfilingIfNeeded() {
   if (profiling_mode == "") {
     AllocationContextTracker::SetCaptureMode(
         AllocationContextTracker::CaptureMode::PSEUDO_STACK);
-#if HAVE_TRACE_STACK_FRAME_POINTERS && \
-    (BUILDFLAG(ENABLE_PROFILING) || !defined(NDEBUG))
+#if (BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) || !defined(NDEBUG))
   } else if (profiling_mode == switches::kEnableHeapProfilingModeNative) {
     // We need frame pointers for native tracing to work, and they are
     // enabled in profiling and debug builds.

build/config/compiler/BUILD.gn

@@ -1381,18 +1381,22 @@ if (is_win) {
 }
 
 config("default_stack_frames") {
-  if (is_posix && !(is_mac || is_ios)) {
-    if (using_sanitizer || enable_profiling || is_debug ||
-        current_cpu == "arm64") {
-      # Explicitly ask for frame pointers, otherwise:
-      # * Stacks may be missing for sanitizer and profiling builds.
-      # * Debug tcmalloc can crash (crbug.com/636489).
-      # * Stacks may be missing for arm64 crash dumps (crbug.com/391706).
+  if (is_posix) {
+    if (enabled_frame_pointers) {
       cflags = [ "-fno-omit-frame-pointer" ]
-    } else if (is_android) {
+    } else {
       cflags = [ "-fomit-frame-pointer" ]
     }
   }
+  # On Windows, the flag to enable framepointers "/Oy-" must always come after
+  # the optimization flag [e.g. "/O2"]. The optimization flag is set by one of
+  # the "optimize" configs, see rest of this file. The ordering that cflags are
+  # applied is well-defined by the GN spec, and there is no way to ensure that
+  # cflags set by "default_stack_frames" is applied after those set by an
+  # "optimize" config. Similarly, there is no way to propagate state from this
+  # config into the "optimize" config. We always apply the "/Oy-" config in the
+  # definition for common_optimize_on_cflags definition, even though this may
+  # not be correct.
 }
 
 # Default "optimization on" config.

build/config/compiler/compiler.gni

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import("//build/config/android/config.gni")
+import("//build/config/arm.gni")
 import("//build/config/chrome_build.gni")
 import("//build/config/chromecast_build.gni")
 import("//build/config/compiler/pgo/pgo.gni")
@@ -65,6 +66,34 @@ declare_args() {
   use_pic = true
 }
 
+# Whether to emit frame pointers by default.
+if (is_mac || is_ios) {
+  enabled_frame_pointers = true
+} else if (is_win) {
+  # 64-bit Windows ABI doesn't support frame pointers.
+  if (target_cpu == "x64") {
+    enabled_frame_pointers = false
+  } else {
+    enabled_frame_pointers = true
+  }
+} else {
+  # Explicitly ask for frame pointers, otherwise:
+  # * Stacks may be missing for sanitizer and profiling builds.
+  # * Debug tcmalloc can crash (crbug.com/636489).
+  # * Stacks may be missing for arm64 crash dumps (crbug.com/391706).
+  enabled_frame_pointers =
+      using_sanitizer || enable_profiling || is_debug || current_cpu == "arm64"
+}
+
+# Unwinding with frame pointers requires that frame pointers are enabled by
+# default for most translation units, and that the architecture isn't thumb, as
+# frame pointers are not correctly emitted for thumb.
+if (enabled_frame_pointers && !(current_cpu == "arm" && arm_use_thumb)) {
+  can_unwind_with_frame_pointers = true
+} else {
+  can_unwind_with_frame_pointers = false
+}
+
 declare_args() {
   # Whether or not the official builds should be built with full WPO. Enabled by
   # default for the PGO and the x64 builds.

chrome/browser/chromeos/libc_close_tracking.cc

@@ -11,6 +11,7 @@
 #include <unordered_map>
 
 #include "base/debug/crash_logging.h"
+#include "base/debug/debugging_flags.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -119,7 +120,7 @@ int CloseOverride(int fd) {
 
   // Capture stack for successful close.
   if (ret == 0) {
-#if HAVE_TRACE_STACK_FRAME_POINTERS && !defined(MEMORY_SANITIZER)
+#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) && !defined(MEMORY_SANITIZER)
     // Use TraceStackFramePointers because the backtrack() based default
     // capturing gets only the last stack frame and is not useful.
     // With the exception of when MSAN is enabled. See comments for why

tools/gn/bootstrap/bootstrap.py

@@ -171,7 +171,8 @@ def build_gn_with_ninja_manually(tempdir, options):
   write_buildflag_header_manually(root_gen_dir, 'base/debug/debugging_flags.h',
       {
           'ENABLE_PROFILING': 'false',
-          'ENABLE_MEMORY_TASK_PROFILER': 'false'
+          'ENABLE_MEMORY_TASK_PROFILER': 'false',
+          'CAN_UNWIND_WITH_FRAME_POINTERS': 'false'
       })
 
   write_build_date_header(root_gen_dir)