Skip to content

Commit

Permalink
Revert "Reland "Reland "Enable extracting unwind table on official bu…
Browse files Browse the repository at this point in the history
…ilds without channel"""

This reverts commit 579e583.

Reason for revert: monochrome_apk_checker.py needs to be updated.
crbug/827937

Original change's description:
> Reland "Reland "Enable extracting unwind table on official builds without channel""
> 
> This reverts commit 2298aca.
> 
> Reason for revert: Remove changes made in monochrome since apk merge script needs to
> be updated first.
> 
> Original change's description:
> > Revert "Reland "Enable extracting unwind table on official builds without channel""
> >
> > This reverts commit 0db71a0.
> >
> > Reason for revert: Monochrome apk merge is failing because the unwind
> > file for both apk has the same name.
> >
> > Original change's description:
> > > Reland "Enable extracting unwind table on official builds without channel"
> > >
> > > This reverts commit a2474b6.
> > >
> > > Reason for revert: The failure was due to parent CL getting reverted
> > > Relanded parent in: https://chromium-review.googlesource.com/c/chromium/src/+/984452
> > >
> > > Original change's description:
> > > > Revert "Enable extracting unwind table on official builds without channel"
> > > >
> > > > This reverts commit eb07cea.
> > > >
> > > > Reason for revert: Windows tryjobs are failing.
> > > > https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2Fwin-msvc-rel%2F81420%2F%2B%2Frecipes%2Fsteps%2Fanalyze%2F0%2Fstdout for an example.
> > > >
> > > > ERROR at //base/BUILD.gn:1802:33: Undefined identifier in string expansion.
> > > >     "CAN_UNWIND_WITH_CFI_TABLE=$can_unwind_with_cfi_table",
> > > >                                 ^------------------------
> > > > "can_unwind_with_cfi_table" is not currently in scope.
> > > > See //BUILD.gn:65:5: which caused the file to be included.
> > > >     "//base:base_perftests",
> > > >     ^----------------------
> > > > GN gen failed: 1
> > > >
> > > > Original change's description:
> > > > > Enable extracting unwind table on official builds without channel
> > > > >
> > > > > This CL enables extraction of unwind tables for chrome when
> > > > > is_official_build is set to true and when android_channel is not set to
> > > > > a release channel. This affects build time only on official builds.
> > > > > Also, Set legacy heap profiler to use unwinder for profiling.
> > > > >
> > > > > TBR=dpranke@chromium.org
> > > > > BUG=819888
> > > > >
> > > > > Change-Id: If9732b5ac1f9dff49c58e88e2ffb6cb387715244
> > > > > Reviewed-on: https://chromium-review.googlesource.com/976944
> > > > > Reviewed-by: Siddhartha S <ssid@chromium.org>
> > > > > Reviewed-by: agrieve <agrieve@chromium.org>
> > > > > Commit-Queue: Siddhartha S <ssid@chromium.org>
> > > > > Cr-Commit-Position: refs/heads/master@{#546425}
> > > >
> > > > TBR=dpranke@chromium.org,ssid@chromium.org,agrieve@chromium.org
> > > >
> > > > Change-Id: Id033094871dfcf74248f631390f15f379d9cd3c7
> > > > No-Presubmit: true
> > > > No-Tree-Checks: true
> > > > No-Try: true
> > > > Bug: 819888
> > > > Reviewed-on: https://chromium-review.googlesource.com/983893
> > > > Reviewed-by: agrieve <agrieve@chromium.org>
> > > > Commit-Queue: agrieve <agrieve@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#546478}
> > >
> > > TBR=dpranke@chromium.org,ssid@chromium.org,agrieve@chromium.org
> > >
> > > Change-Id: I100f540c8d5bc9f59b9631ea207c7f290dc42e96
> > > No-Presubmit: true
> > > No-Tree-Checks: true
> > > No-Try: true
> > > Bug: 819888
> > > Reviewed-on: https://chromium-review.googlesource.com/985112
> > > Commit-Queue: Siddhartha S <ssid@chromium.org>
> > > Reviewed-by: Siddhartha S <ssid@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#547196}
> >
> > TBR=dpranke@chromium.org,ssid@chromium.org,agrieve@chromium.org
> >
> > Change-Id: Ifa59a5882b18c2634bcf96c2c1fc58b09d7f303f
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: 819888
> > Reviewed-on: https://chromium-review.googlesource.com/989153
> > Reviewed-by: Siddhartha S <ssid@chromium.org>
> > Commit-Queue: Siddhartha S <ssid@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#547332}
> 
> TBR=dpranke@chromium.org,ssid@chromium.org,agrieve@chromium.org,ctzsm@chromium.org
> 
> Change-Id: I7750c70e52b55ccfbf1e2457a19fee4852575e59
> Bug: 819888
> Reviewed-on: https://chromium-review.googlesource.com/989192
> Commit-Queue: Siddhartha S <ssid@chromium.org>
> Reviewed-by: Siddhartha S <ssid@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547361}

TBR=dpranke@chromium.org,ssid@chromium.org,agrieve@chromium.org,ctzsm@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 819888
Change-Id: I064a5bdae54fa1552eee14ee72d08f15cf081a63
Reviewed-on: https://chromium-review.googlesource.com/990092
Reviewed-by: Siddhartha S <ssid@chromium.org>
Commit-Queue: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547490}
  • Loading branch information
ssiddhartha authored and Commit Bot committed Apr 2, 2018
1 parent b3e086e commit c36e45c
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 56 deletions.
1 change: 0 additions & 1 deletion base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1808,7 +1808,6 @@ buildflag_header("debugging_buildflags") {
"ENABLE_PROFILING=$enable_profiling",
"CAN_UNWIND_WITH_FRAME_POINTERS=$can_unwind_with_frame_pointers",
"UNSAFE_DEVELOPER_BUILD=$is_unsafe_developer_build",
"CAN_UNWIND_WITH_CFI_TABLE=$can_unwind_with_cfi_table",
]
}

Expand Down
17 changes: 3 additions & 14 deletions base/trace_event/heap_profiler_allocation_context_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@
#include "base/threading/platform_thread.h"
#include "base/threading/thread_local_storage.h"
#include "base/trace_event/heap_profiler_allocation_context.h"
#include "build/build_config.h"

#if defined(OS_ANDROID) && BUILDFLAG(CAN_UNWIND_WITH_CFI_TABLE)
#include "base/trace_event/cfi_backtrace_android.h"
#endif

#if defined(OS_LINUX) || defined(OS_ANDROID)
#include <sys/prctl.h>
Expand Down Expand Up @@ -219,26 +214,20 @@ bool AllocationContextTracker::GetContextSnapshot(AllocationContext* ctx) {
// kMaxFrameCount + 1 frames, so that we know if there are more frames
// than our backtrace capacity.
#if !defined(OS_NACL) // We don't build base/debug/stack_trace.cc for NaCl.
#if defined(OS_ANDROID) && BUILDFLAG(CAN_UNWIND_WITH_CFI_TABLE)
const void* frames[Backtrace::kMaxFrameCount + 1];
static_assert(arraysize(frames) >= Backtrace::kMaxFrameCount,
"not requesting enough frames to fill Backtrace");
size_t frame_count = CFIBacktraceAndroid::GetInstance()->Unwind(
frames, arraysize(frames));
#elif BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
#if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
const void* frames[Backtrace::kMaxFrameCount + 1];
static_assert(arraysize(frames) >= Backtrace::kMaxFrameCount,
"not requesting enough frames to fill Backtrace");
size_t frame_count = debug::TraceStackFramePointers(
frames, arraysize(frames),
1 /* exclude this function from the trace */);
#else
#else // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
// Fall-back to capturing the stack with base::debug::StackTrace,
// which is likely slower, but more reliable.
base::debug::StackTrace stack_trace(Backtrace::kMaxFrameCount + 1);
size_t frame_count = 0u;
const void* const* frames = stack_trace.Addresses(&frame_count);
#endif
#endif // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)

// If there are too many frames, keep the ones furthest from main().
size_t backtrace_capacity = backtrace_end - backtrace;
Expand Down
16 changes: 2 additions & 14 deletions base/trace_event/memory_dump_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,8 @@

#if defined(OS_ANDROID)
#include "base/trace_event/java_heap_dump_provider_android.h"

#if BUILDFLAG(CAN_UNWIND_WITH_CFI_TABLE)
#include "base/trace_event/cfi_backtrace_android.h"
#endif

#endif // defined(OS_ANDROID)

namespace base {
namespace trace_event {

Expand Down Expand Up @@ -278,15 +273,8 @@ bool MemoryDumpManager::EnableHeapProfiling(HeapProfilingMode profiling_mode) {
break;

case kHeapProfilingModeNative:
#if defined(OS_ANDROID) && BUILDFLAG(CAN_UNWIND_WITH_CFI_TABLE)
{
bool can_unwind =
CFIBacktraceAndroid::GetInstance()->can_unwind_stack_frames();
DCHECK(can_unwind);
}
#endif
// If we don't have frame pointers and unwind tables then native tracing
// falls-back to using base::debug::StackTrace, which may be slow.
// If we don't have frame pointers then native tracing falls-back to
// using base::debug::StackTrace, which may be slow.
AllocationContextTracker::SetCaptureMode(
AllocationContextTracker::CaptureMode::NATIVE_STACK);
break;
Expand Down
8 changes: 4 additions & 4 deletions build/config/android/extract_unwind_tables.gni
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ template("unwind_table_asset") {
"--dump_syms_path",
rebase_path("$root_out_dir/dump_syms", root_build_dir),
]

deps = invoker.deps
deps += [ "//third_party/breakpad:dump_syms" ]
deps = [
":${invoker.library_target}",
"//third_party/breakpad:dump_syms",
]
}

android_assets(target_name) {
if (defined(invoker.testonly)) {
testonly = invoker.testonly
Expand Down
20 changes: 0 additions & 20 deletions chrome/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import("//tools/v8_context_snapshot/v8_context_snapshot.gni")
import("channel.gni")
import("java_sources.gni")
import("static_initializers.gni")
import("//build/config/android/extract_unwind_tables.gni")

manifest_package = "org.chromium.chrome"

Expand Down Expand Up @@ -734,22 +733,6 @@ java_group("chrome_public_non_pak_assets") {
}
}

# Enable stack unwinding only for local official builds. Enabling on all local
# builds would increase build time for developers. Choosing a release channel
# should be done based on the size of the unwind file and performance of
# unwinding.
_add_unwind_tables_in_apk = can_unwind_with_cfi_table && is_android &&
is_official_build && android_channel == "default"

if (_add_unwind_tables_in_apk) {
unwind_table_asset("chrome_public_unwind_assets") {
library_target = "chrome"
deps = [
":libchrome",
]
}
}

android_assets("chrome_public_pak_assets") {
sources = [
"$root_out_dir/chrome_100_percent.pak",
Expand All @@ -761,9 +744,6 @@ android_assets("chrome_public_pak_assets") {
":chrome_public_locale_pak_assets",
"//chrome:packed_resources",
]
if (_add_unwind_tables_in_apk) {
deps += [ ":chrome_public_unwind_assets" ]
}
}

# This target is separate from chrome_public_pak_assets because it does not
Expand Down
3 changes: 0 additions & 3 deletions testing/test.gni
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ template("test") {
unwind_table_asset(_unwind_table_asset_name) {
testonly = true
library_target = _library_target
deps = [
":$_library_target",
]
}
}

Expand Down

0 comments on commit c36e45c

Please sign in to comment.