Revert "[Sampling profiler] Fix and test NativeUnwinderAndroid module extents"

This reverts commit b95277ba.

Reason for revert: suspected of causing test failures on Marshmallow
Tablet Tester and android-asan

Original change's description:
> [Sampling profiler] Fix and test NativeUnwinderAndroid module extents
> 
> Reuses the ModuleCache POSIX module implementation for Android ELF
> modules, which fixes the incorrect start and end addresses previously
> generated by AndroidModule. Renames AndroidModule to NonElfModule.
> Removes tests specific to the previous implementation.
> 
> Also includes a latent 64-bit bug fix where the register copy would
> only copy half the registers.
> 
> Bug: 1004855
> Change-Id: I3b26fac427043b2022b1d35b5c985e468ce35692
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253221
> Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
> Commit-Queue: Mike Wittman <wittman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#783689}

TBR=wittman@chromium.org,etiennep@chromium.org

Change-Id: I48aca7285e572eab5147ef584dda01eeabfb2008
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1004855, 1101023
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2274327Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784062}

base/profiler/native_unwinder_android.cc

@@ -18,6 +18,8 @@
 #include "base/profiler/module_cache.h"
 #include "base/profiler/native_unwinder.h"
 #include "base/profiler/profile_builder.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
 #include "build/build_config.h"
 
 #if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)
@@ -31,21 +33,46 @@
 namespace base {
 namespace {
 
-class NonElfModule : public ModuleCache::Module {
+// Returns the hex string build id given the binary build id from MapInfo.
+// Returns the empty string if no build id is present.
+//
+// Build IDs follow a cross-platform format consisting of two fields
+// concatenated together:
+// - the module's unique id encoded as a hex string, and
+// - the age suffix for incremental builds.
+//
+// On POSIX, the unique id comes from the ELF binary's .note.gnu.build-id
+// section. The age field is always 0.
+std::string EncodeBuildID(StringPiece map_info_build_id) {
+  if (map_info_build_id.empty())
+    return std::string();
+
+  return HexEncode(map_info_build_id.data(), map_info_build_id.size()) + "0";
+}
+
+// We assume this is a file-backed region if the name looks like an absolute
+// path, and return the basename. Otherwise we assume the region is not
+// associated with a file and return the full name string.
+FilePath CreateDebugBasename(StringPiece map_info_name) {
+  const FilePath path = FilePath(map_info_name);
+  return !map_info_name.empty() && map_info_name[0] == '/' ? path.BaseName()
+                                                           : path;
+}
+
+class AndroidModule : public ModuleCache::Module {
  public:
-  NonElfModule(unwindstack::MapInfo* map_info)
+  AndroidModule(unwindstack::MapInfo* map_info)
       : start_(map_info->start),
-        size_(map_info->end - start_),
-        map_info_name_(map_info->name) {}
-  ~NonElfModule() override = default;
+        size_(map_info->end - map_info->start),
+        build_id_(EncodeBuildID(map_info->GetBuildID())),
+        debug_basename_(CreateDebugBasename(map_info->name)) {}
+  ~AndroidModule() override = default;
 
   uintptr_t GetBaseAddress() const override { return start_; }
 
-  std::string GetId() const override { return std::string(); }
+  std::string GetId() const override { return build_id_; }
 
-  FilePath GetDebugBasename() const override {
-    return FilePath(map_info_name_);
-  }
+  FilePath GetDebugBasename() const override { return debug_basename_; }
 
   // Gets the size of the module.
   size_t GetSize() const override { return size_; }
@@ -53,10 +80,10 @@ class NonElfModule : public ModuleCache::Module {
   // True if this is a native module.
   bool IsNative() const override { return true; }
 
- private:
   const uintptr_t start_;
   const size_t size_;
-  const std::string map_info_name_;
+  const std::string build_id_;
+  const FilePath debug_basename_;
 };
 
 std::unique_ptr<unwindstack::Regs> CreateFromRegisterContext(
@@ -77,10 +104,10 @@ void CopyToRegisterContext(unwindstack::Regs* regs,
                            RegisterContext* thread_context) {
 #if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)
   memcpy(reinterpret_cast<void*>(&thread_context->arm_r0), regs->RawData(),
-         unwindstack::ARM_REG_LAST * sizeof(uintptr_t));
+         unwindstack::ARM_REG_LAST * sizeof(uint32_t));
 #elif defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_64_BITS)
   memcpy(reinterpret_cast<void*>(&thread_context->regs[0]), regs->RawData(),
-         unwindstack::ARM64_REG_LAST * sizeof(uintptr_t));
+         unwindstack::ARM64_REG_LAST * sizeof(uint32_t));
 #else   // #if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)
   NOTREACHED();
 #endif  // #if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)
@@ -219,29 +246,12 @@ UnwindResult NativeUnwinderAndroid::TryUnwind(RegisterContext* thread_context,
 void NativeUnwinderAndroid::AddInitialModulesFromMaps(
     const unwindstack::Maps& memory_regions_map,
     ModuleCache* module_cache) {
-  // The effect of this loop is to create modules for the executable regions in
-  // the memory map. Regions composing a mapped ELF file are handled specially
-  // however: for just one module extending from the ELF base address to the
-  // *last* executable region backed by the file is implicitly created by
-  // ModuleCache. This avoids duplicate module instances covering the same
-  // in-memory module in the case that a module has multiple mmapped executable
-  // regions.
-  for (const std::unique_ptr<unwindstack::MapInfo>& region :
-       memory_regions_map) {
+  for (const auto& region : memory_regions_map) {
+    // Only add executable regions.
     if (!(region->flags & PROT_EXEC))
       continue;
-
-    // Use the standard ModuleCache POSIX module representation for ELF files.
-    // This call returns the containing ELF module for the region, creating it
-    // if it doesn't exist.
-    if (module_cache->GetModuleForAddress(
-            static_cast<uintptr_t>(region->start))) {
-      continue;
-    }
-
-    // Non-ELF modules are represented with NonElfModule.
     module_cache->AddCustomNativeModule(
-        std::make_unique<NonElfModule>(region.get()));
+        std::make_unique<AndroidModule>(region.get()));
   }
 }
 
@@ -257,7 +267,7 @@ void NativeUnwinderAndroid::EmitDexFrame(uintptr_t dex_pc,
     // AddInitialModulesFromMaps().
     unwindstack::MapInfo* map_info = memory_regions_map_->Find(dex_pc);
     if (map_info) {
-      auto new_module = std::make_unique<NonElfModule>(map_info);
+      auto new_module = std::make_unique<AndroidModule>(map_info);
       module = new_module.get();
       module_cache->AddCustomNativeModule(std::move(new_module));
     }

base/profiler/native_unwinder_android_unittest.cc

@@ -6,12 +6,9 @@
 
 #include <sys/mman.h>
 
-#include <inttypes.h>
 #include <stdio.h>  // For printf address.
 #include <string.h>
-#include <algorithm>
 #include <iterator>
-#include <vector>
 
 #include "base/android/build_info.h"
 #include "base/android/jni_android.h"
@@ -23,7 +20,6 @@
 #include "base/profiler/stack_sampler.h"
 #include "base/profiler/stack_sampling_profiler_test_util.h"
 #include "base/profiler/thread_delegate_posix.h"
-#include "base/strings/stringprintf.h"
 #include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,11 +30,6 @@ namespace base {
 
 namespace {
 
-bool CompareModulesByBaseAddress(const ModuleCache::Module* a,
-                                 const ModuleCache::Module* b) {
-  return a->GetBaseAddress() < b->GetBaseAddress();
-}
-
 // Add a MapInfo with the provided values to |maps|.
 void AddMapInfo(uint64_t start,
                 uint64_t end,
@@ -396,8 +387,25 @@ TEST(NativeUnwinderAndroidTest, UnwindStackMemoryTest) {
   check_read_succeeds(end - 1, 1);
 }
 
-// Checks the debug basename is the whole name for a non-ELF module.
-TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForNonElf) {
+// Checks the debug basename for a module with a path name.
+TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForPath) {
+  unwindstack::Maps maps;
+
+  AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "/usr/lib/foo.so",
+             {0xAA}, maps);
+
+  ModuleCache module_cache;
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(maps, &module_cache);
+
+  std::vector<const ModuleCache::Module*> modules = module_cache.GetModules();
+
+  ASSERT_EQ(1u, modules.size());
+  EXPECT_EQ("foo.so", modules[0]->GetDebugBasename().value());
+}
+
+// Checks the debug basename is the whole name for a module with a non-path
+// name.
+TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForNonPath) {
   unwindstack::Maps maps;
 
   AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "[foo / bar]", {0xAA},
@@ -412,48 +420,37 @@ TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForNonElf) {
   EXPECT_EQ("[foo / bar]", modules[0]->GetDebugBasename().value());
 }
 
-// Checks that modules are only created for executable memory regions.
-TEST(NativeUnwinderAndroidTest, ModulesCreatedOnlyForExecutableRegions) {
+// Checks that the specified build id is returned.
+TEST(NativeUnwinderAndroidTest, ModuleId) {
   unwindstack::Maps maps;
-  AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "[a]", {0xAA}, maps);
-  AddMapInfo(0x2000u, 0x3000u, 0u, PROT_READ, "[b]", {0xAB}, maps);
-  AddMapInfo(0x3000u, 0x4000u, 0u, PROT_READ | PROT_EXEC, "[c]", {0xAC}, maps);
+
+  AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "/lib/foo.so",
+             {0x12, 0x34, 0x56, 0x78, 0x90, 0xAB, 0xCD, 0xEF}, maps);
 
   ModuleCache module_cache;
   NativeUnwinderAndroid::AddInitialModulesFromMaps(maps, &module_cache);
 
   std::vector<const ModuleCache::Module*> modules = module_cache.GetModules();
-  std::sort(modules.begin(), modules.end(), CompareModulesByBaseAddress);
 
-  ASSERT_EQ(2u, modules.size());
-  EXPECT_EQ(0x1000u, modules[0]->GetBaseAddress());
-  EXPECT_EQ(0x3000u, modules[1]->GetBaseAddress());
+  ASSERT_EQ(1u, modules.size());
+  // The id should have a '0' age field appended.
+  EXPECT_EQ("1234567890ABCDEF0", modules[0]->GetId());
 }
 
-// Checks that module address ranges don't overlap.
-TEST(NativeUnwinderAndroidTest, NonOverlappingModules) {
+// Checks that an empty module id has no age field appended.
+TEST(NativeUnwinderAndroidTest, EmptyModuleId) {
+  unwindstack::Maps maps;
+
+  AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "/lib/foo.so",
+             std::string(), maps);
+
   ModuleCache module_cache;
-  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(*maps, &module_cache);
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(maps, &module_cache);
 
   std::vector<const ModuleCache::Module*> modules = module_cache.GetModules();
-  std::sort(modules.begin(), modules.end(), CompareModulesByBaseAddress);
-  auto loc = std::adjacent_find(
-      modules.begin(), modules.end(),
-      [](const ModuleCache::Module* m1, const ModuleCache::Module* m2) {
-        return m2->GetBaseAddress() < m1->GetBaseAddress() + m1->GetSize();
-      });
-
-  const auto describe_module = [](const ModuleCache::Module* module) {
-    return StringPrintf(
-        "id \"%s\", debug basename \"%s\" at [0x%" PRIxPTR ", 0x%" PRIxPTR ")",
-        module->GetId().c_str(), module->GetDebugBasename().value().c_str(),
-        module->GetBaseAddress(), module->GetBaseAddress() + module->GetSize());
-  };
 
-  EXPECT_EQ(modules.end(), loc) << "module overlap found between\n"
-                                << "  " << describe_module(*loc) << " and \n"
-                                << "  " << describe_module(*std::next(loc));
+  ASSERT_EQ(1u, modules.size());
+  EXPECT_EQ(std::string(), modules[0]->GetId());
 }
 
 // ModuleCache::GetModuleForAddress() is not implemented for 64-bit arm.
@@ -466,9 +463,8 @@ TEST(NativeUnwinderAndroidTest, NonOverlappingModules) {
 // state created by the ModuleCache. Checks the module for a system library.
 TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
   ModuleCache unwinder_module_cache;
-  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(*maps,
-                                                   &unwinder_module_cache);
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(
+      *NativeUnwinderAndroid::CreateMaps(), &unwinder_module_cache);
 
   const uintptr_t c_library_function_address =
       reinterpret_cast<uintptr_t>(&printf);
@@ -483,12 +479,11 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
       reference_module_cache.GetModuleForAddress(c_library_function_address);
   ASSERT_NE(nullptr, reference_module);
 
-  EXPECT_EQ(reference_module->GetBaseAddress(),
-            unwinder_module->GetBaseAddress());
+  // TODO(https://crbug.com/1004855): Fix base address and size discrepancies
+  // and add checks.
   EXPECT_EQ(reference_module->GetId(), unwinder_module->GetId());
   EXPECT_EQ(reference_module->GetDebugBasename(),
             unwinder_module->GetDebugBasename());
-  EXPECT_EQ(unwinder_module->GetSize(), reference_module->GetSize());
 }
 
 // ModuleCache::GetModuleForAddress() is not implemented for 64-bit arm.
@@ -502,9 +497,8 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
 // library.
 TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_ChromeLibrary) {
   ModuleCache unwinder_module_cache;
-  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(*maps,
-                                                   &unwinder_module_cache);
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(
+      *NativeUnwinderAndroid::CreateMaps(), &unwinder_module_cache);
 
   const uintptr_t chrome_function_address =
       reinterpret_cast<uintptr_t>(&CaptureScenario);
@@ -525,7 +519,7 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_ChromeLibrary) {
   EXPECT_EQ(reference_module->GetId(), unwinder_module->GetId());
   EXPECT_EQ(reference_module->GetDebugBasename(),
             unwinder_module->GetDebugBasename());
-  EXPECT_EQ(unwinder_module->GetSize(), reference_module->GetSize());
+  // TODO(https://crbug.com/1004855): Fix size discrepancy and add check.
 }
 
 }  // namespace base