Revert "Reland "Reland "Add toolchains without PartitionAlloc-Everywhere for dump_syms et al"""
This reverts commit 5e6def5bd2aa9cdbf69608a8edc32b5de696c091.
Reason for revert: official/win-clang is still broken:
https://ci.chromium.org/ui/p/chrome/builders/official/win-clang/8651/overview
Original change's description:
> Reland "Reland "Add toolchains without PartitionAlloc-Everywhere for dump_syms et al""
>
> This is a reland of commit 818e126f4350095dd0a54566ded107f0a5065f6f
> Which was a reland of commit 38c00784bc98a5dc885b06f5a0e738386c5f7df7
>
> When PartitionAlloc is linked into an executable, it takes over the
> system allocator (malloc, new, etc), which is called PartitionAlloc-
> Everywhere (or PA-E). When this occurs in dump_syms, we see that PA
> hits OOM and causes dump_syms to crash while generating the symbols
> for chrome. It's not at all clear why PA hits OOM but the system
> allocator does not, it occurs during construction of a std::string (on
> my machine anyway when I am running it in gdb, maybe elsewhere on bots).
> This happens on all platforms that we run dump_syms on as part of the
> official build: on linux and on mac, building for at least linux,
> android, chromeos, and mac. See also crbug.com/345514993.
>
> So we want to build dump_syms and other breakpad executables in a way
> that uses the system allocator. To do that we need to disable the
> use_partition_alloc_as_malloc GN variable. As this variable is global,
> we need a separate toolchain in order to disable it.
>
> We introduce a new toolchain with the suffix `_with_system_allocator`
> that can be used for this purpose. Initially we intended to use the
> Rust host build tools toolchain for this purpose, however we require
> careful naming to avoid toolchain collisions. For instance if building
> on a Linux x64 machine with an Other x64 target, we can have two
> toolchains:
> - default_toolchain: //build/toolchain/other:clang_x64
> - host_toolchain: //build/toolchain/other:clang_x64
>
> While these have different labels, it is the name at the end that is
> used as their output directory (this is hardcoded in GN). But they
> avoid colliding because the default toolchain is not placed in a
> subdirectory and uses the `root_build_dir`. However when we add another
> toolchain with them, they both get subdirs and collide:
> - for target: //build/toolchain/other:clang_x64_with_system_allocator
> - for host: //build/toolchain/linux:clang_x64_with_system_allocator
>
> Now both toolchains try to write to the clang_x64_with_system_allocator
> directory which causes errors. To avoid this, we actually make two
> toolchains per toolchain, one with a `host_` tag inside it.
> - target: //build/toolchain/other:clang_x64_with_system_allocator
> - unused: //build/toolchain/linux:clang_x64_with_system_allocator
> - unused: //build/toolchain/other:clang_x64_host_with_system_allocator
> - host: //build/toolchain/linux:clang_x64_host_with_system_allocator
>
> Then, when building for the host we choose the `host_` variety, which
> is specified in the `host_system_allocator_toolchain variable`. And
> when building for default target, we choose the non-`host_` one, which
> is specified in the `default_system_allocator_toolchain` variable.
>
> More clever strategies that try to avoid creating the unused toolchains
> above do not seem possible. Inside the toolchain-creating template,
> it is not clear how to determine which toolchain is being created, as
> the get_label_info() function on `target_name` does not produce
> anything that matches exactly with the string in `default_toolchain` or
> `host_toolchain`. We also tried using the current_cpu and current_os,
> however the `toolchain_args.current_os` is not actually set correctly in
> the default toolchain when targeting ChromeOS. The current_os variable
> is "chromeos" but inside the toolchain_args, it is "linux". So we just
> make extra toolchains (which can't be used or they'd make build errors)
> and we don't refer to them from the `host_system_allocator_toolchain`
> and `default_system_allocator_toolchain` variables, which makes them
> effectively inaccessible.
>
> In the process we learnt many things about how the breakpad executables
> are built. When you build them for the default toolchain, such as by
> building `//third_party/breakpad:dump_syms`, it redirects to the *host*
> toolchain on many platforms, but not on all platforms. This ends up
> putting a binary that may not work on the target machine in the
> `root_build_dir` which is highly unusual, but it is required by our
> testing scripts/infra.
>
> The key insight added here is that the toolchain that it should be
> built with is the platform from where the tests on the target will be
> *launched*. On Android, iOS, and ChromeOS, the tests are launched from
> the host machine and that's where the breakpad executables are run. We
> encode this explicitly in the breakpad GN file.
>
> One additional exception is that the breakpad tools do not build on
> Windows ARM, so when building on Windows x64 for Windows ARM, while
> the tests are launched from the ARM machine, we target the host x64
> machine still. This relies on the ARM machine being able to run the
> x64 binaries through emulation.
>
> There's no change here in how the breakpad binaries are built, but it
> is now more explicitly encoded and documented. What did change is that
> since we use a separate toolchain for building these tools, we also
> turn off component build in them. This allows us to replace the use
> of symlinks with copying (or hardlinking) the binaries from the
> toolchain's root directory up to the root_build_dir. This enables
> support for building these tools in the default_toolchain on Windows,
> something which was not possible before.
>
> Additional fixes from the original CL:
>
> MSAN is disabled in the toolchain with the system allocator as we only
> support MSAN in the default toolchain. If another toolchain has MSAN
> enabled it will try to also generate the MSAN instrumented libraries
> in the default toolchain's directory and they collide. This is similar
> to the rust host build tools toolchain, but there we disable all
> sanitizers. For the system allocator toolchain, we disable MSAN but
> retain the ability to build these tools with ASAN or UBSAN if needed.
>
> Angle's GN generation is fixed by not setting the PA variable directly
> from the toolchain. We add a variable in toolchain.gni that is always
> present, and set that. Then in the PA gni files, we check for that
> variable before enabling PA-Everywhere (and BRP, etc).
>
> Devtools standalone overrides BUILDCONFIG.gn but was not re-defining
> the TESTONLY_AND_VISIBILITY variable, so this is done in
> https://chrome-internal-review.googlesource.com/c/devtools/devtools-internal/+/7412037
>
> iOS official internal builders are now using the path to the
> root_build_dir for its dump_syms exe path from
> https://chrome-internal-review.googlesource.com/c/chrome/ios_internal/+/7411376
> and it expects the executable to be for the host. A TODO is added
> in the breakpad BUILD.gn file regarding cross-compiling for a
> different mac machine architecture that will upload/launch tests to
> the iOS device.
>
> Mac and Windows internal official builders are fixed by having the
> symupload tool depended on and built for the default toolchain so that
> it's present in the root_build_dir, but making this binary always
> redirect through the host_system_allocator_toolchain. The symupload
> binary is only run on official builders, it's not part of test
> failure reporting like dump_syms.
>
> Clank orderfile generator had a GN error due to PA-E being off but
> BRP being enabled. This is resolved by the fix for Angle, by turning
> off all PA-E and BRP related stuff when the toolchain turns off PA-E.
> See https://crbug.com/347976629.
>
> Bug: 345514993, b/342251590, 347976629
> Change-Id: Id953eb91ee73486c8e4f7dfc2bffe12fedd8d629
> Cq-Include-Trybots: luci.chromium.try:linux-official,android-official,win-official,mac-official
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_msan_rel_ng
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_asan_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5645742
> Commit-Queue: danakj <[email protected]>
> Reviewed-by: Hans Wennborg <[email protected]>
> Owners-Override: danakj <[email protected]>
> Reviewed-by: Mark Mentovai <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1318726}
Bug: 345514993, b/342251590, 347976629
Change-Id: Idf770547eed6a319ea21b79518d3926bd48ac823
Cq-Include-Trybots: luci.chromium.try:linux-official,android-official,win-official,mac-official
Cq-Include-Trybots: luci.chromium.try:linux_chromium_msan_rel_ng
Cq-Include-Trybots: luci.chromium.try:linux_chromium_asan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5655811
Commit-Queue: Will Yeager <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Keybo Qian <[email protected]>
Reviewed-by: Junji Watanabe <[email protected]>
Owners-Override: Prudhvikumar Bommana <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1319570}
diff --git a/BUILD.gn b/BUILD.gn
index c7a135b..97ec7e6 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -603,10 +603,10 @@
"//mojo:mojo_perftests",
"//services/service_manager/public/cpp",
"//testing/gmock:gmock_main",
- "//third_party/breakpad:dump_syms($host_system_allocator_toolchain)",
- "//third_party/breakpad:microdump_stackwalk($host_system_allocator_toolchain)",
- "//third_party/breakpad:minidump_dump($host_system_allocator_toolchain)",
- "//third_party/breakpad:minidump_stackwalk($host_system_allocator_toolchain)",
+ "//third_party/breakpad:dump_syms($host_toolchain)",
+ "//third_party/breakpad:microdump_stackwalk($host_toolchain)",
+ "//third_party/breakpad:minidump_dump($host_toolchain)",
+ "//third_party/breakpad:minidump_stackwalk($host_toolchain)",
]
}
@@ -628,7 +628,7 @@
if (is_mac) {
deps += [
- "//third_party/breakpad:dump_syms($host_system_allocator_toolchain)",
+ "//third_party/breakpad:dump_syms",
# The following are accessibility API tools.
"//tools/accessibility/inspect:ax_dump_events",
@@ -672,15 +672,8 @@
host_os == "win") {
deps += [ "//chrome/test/mini_installer:mini_installer_tests" ]
}
- }
-
- if (!is_fuchsia) {
- # The official builders use this binary from the default toolchain's
- # output directory after building in order to upload the symbols of that
- # binary. They build the binary like `ninja symupload` which requires the
- # target to be a dependency in the default_toolchain from `gn_all` for the
- # name to resolve.
- deps += [ "//third_party/breakpad:symupload" ]
+ } else if (!is_android && !is_ios && !is_fuchsia) {
+ deps += [ "//third_party/breakpad:symupload($host_toolchain)" ]
}
if (is_cast_android || is_castos || (is_fuchsia && enable_cast_receiver)) {
@@ -985,7 +978,7 @@
"//net:net_unittests",
"//printing:printing_unittests",
"//sql:sql_unittests",
- "//third_party/breakpad:symupload",
+ "//third_party/breakpad:symupload($host_toolchain)",
"//ui/base:ui_base_unittests",
"//ui/gfx:gfx_unittests",
"//ui/touch_selection:ui_touch_selection_unittests",
@@ -1013,7 +1006,7 @@
"//media/capture:capture_unittests",
"//sandbox/linux:chrome_sandbox",
"//sandbox/linux:sandbox_linux_unittests",
- "//third_party/breakpad:minidump_stackwalk($host_system_allocator_toolchain)",
+ "//third_party/breakpad:minidump_stackwalk($host_toolchain)",
"//third_party/dawn/src/dawn/tests:dawn_end2end_tests",
"//third_party/dawn/src/dawn/tests:dawn_unittests",
"//ui/ozone:ozone_integration_tests",
@@ -1187,40 +1180,28 @@
if (is_android) {
data_deps += [
"//third_party/breakpad:breakpad_unittests",
- "//tools/android/forwarder2",
-
- # Using the target toolchain for this tool, as it's run during tests not
- # during the build. This places a symlink in the root_build_dir for
- # scrips to use.
"//third_party/breakpad:dump_syms",
"//third_party/breakpad:microdump_stackwalk",
"//third_party/breakpad:minidump_dump",
"//third_party/breakpad:minidump_stackwalk",
"//third_party/breakpad:symupload",
+ "//tools/android/forwarder2",
]
} else {
data_deps += [ "//content/web_test:web_test_common_mojom_js_data_deps" ]
}
if (!is_win && !is_android) {
- # Using the default toolchain for this tool, as it's run during tests not
- # during the build. This places a symlink in the root_build_dir for scrips
- # to use.
- data_deps += [ "//third_party/breakpad:minidump_stackwalk" ]
+ data_deps +=
+ [ "//third_party/breakpad:minidump_stackwalk($host_toolchain)" ]
}
if (is_mac) {
- # Using the default toolchain for this tool, as it's run during tests not
- # during the build, and on Mac we support cross-building from a different
- # architecture.
- data_deps += [ "//third_party/breakpad:dump_syms" ]
+ data_deps += [ "//third_party/breakpad:dump_syms($host_toolchain)" ]
}
if (is_linux || is_chromeos) {
- # Using the default toolchain for this tool, as it's run during tests not
- # during the build. This places a symlink in the root_build_dir for scrips
- # to use.
- data_deps += [ "//third_party/breakpad:dump_syms" ]
+ data_deps += [ "//third_party/breakpad:dump_syms($host_toolchain)" ]
}
if (is_fuchsia) {
@@ -1684,7 +1665,8 @@
}
if (!is_win) {
- data_deps += [ "//third_party/breakpad:minidump_stackwalk($host_system_allocator_toolchain)" ]
+ data_deps +=
+ [ "//third_party/breakpad:minidump_stackwalk($host_toolchain)" ]
}
}
