From ff51422d9ff82bb21086a1a7adb2dcb870b596db Mon Sep 17 00:00:00 2001 From: Egor Pasko Date: Thu, 18 Nov 2021 13:41:00 +0000 Subject: [PATCH] android: reachedcode: avoid a global std::atomic In C++20 constructing an std::atomic is not 'trivial'. The various reasons for that are reachable from this discussion: [1]. The change will prevent the array from being put to BSS and be cheap when unused. Make the array a regular uint32_t array and refer to it as an array of std::atomic later (using reinterpret_cast). The std::atomic has the standard layout by the spec, making it hard to imagine an implementation of C++ that would construct the std::atomic to anything different than (uint32_t)0. The Linux/Android memory page zero-fill is atomic and can only be done once. Therefore the new behavior should be equivalent to the current one. The code generated by Clang today is identical with/without this patch. This large chunk of zeroes in BSS still _may_ have a performance or code size impact because of increased immediates in relative references to .data and .bss. Resolving it may be even more controversial, hence suggesting to do it only if there is a confirmed regression. Disclaimer: I know this is .. bad. And I feel bad. [1] Discussion: : In pre-C++20 mode, the constructor should be trivial https://github.com/microsoft/STL/issues/661 Bug: None Change-Id: I06798f225bd557844072d154d47ace2d85606df5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3282196 Reviewed-by: Alex Ilin Commit-Queue: Egor Pasko Cr-Commit-Position: refs/heads/main@{#943056} --- base/android/reached_addresses_bitset.cc | 27 +++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/base/android/reached_addresses_bitset.cc b/base/android/reached_addresses_bitset.cc index 295635be5..789ccda50 100644 --- a/base/android/reached_addresses_bitset.cc +++ b/base/android/reached_addresses_bitset.cc @@ -14,18 +14,35 @@ namespace android { namespace { constexpr size_t kBitsPerElement = sizeof(uint32_t) * 8; -#if BUILDFLAG(SUPPORTS_CODE_ORDERING) +// Below an array of uint32_t in BSS is introduced and then casted to an array +// of std::atomic. In C++20 constructing an std::atomic is not +// 'trivial'. See https://github.com/microsoft/STL/issues/661 for reasons of +// this change in the standard. +// +// Assert that both types have the same size. The sizes do not have to match +// according to a note in [atomics.types.generic] in C++17. With this assertion +// in place it is unlikely that the constructor produces the value other than +// (uint32_t)0. +static_assert(sizeof(uint32_t) == sizeof(std::atomic), ""); + +// Keep the array in BSS only for non-official builds to avoid potential harm to +// data locality and unspecified behavior from the reinterpret_cast below. In +// order to start new experiments with base::Feature(ReachedCodeProfiler) on +// Canary/Dev this array will need to be reintroduced to official builds. +#if BUILDFLAG(SUPPORTS_CODE_ORDERING) && !defined(OFFICIAL_BUILD) // Enough for 1 << 29 bytes of code, 512MB. constexpr size_t kTextBitfieldSize = 1 << 20; -std::atomic g_text_bitfield[kTextBitfieldSize]; +uint32_t g_text_bitfield[kTextBitfieldSize]; #endif } // namespace // static ReachedAddressesBitset* ReachedAddressesBitset::GetTextBitset() { -#if BUILDFLAG(SUPPORTS_CODE_ORDERING) - static ReachedAddressesBitset text_bitset(kStartOfText, kEndOfText, - g_text_bitfield, kTextBitfieldSize); +#if BUILDFLAG(SUPPORTS_CODE_ORDERING) && !defined(OFFICIAL_BUILD) + static ReachedAddressesBitset text_bitset( + kStartOfText, kEndOfText, + reinterpret_cast*>(g_text_bitfield), + kTextBitfieldSize); return &text_bitset; #else return nullptr;