[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/+/2253221Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783689}

base/profiler/native_unwinder_android.cc

@@ -18,8 +18,6 @@
 #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)
@@ -33,46 +31,21 @@
 namespace base {
 namespace {
 
-// 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 {
+class NonElfModule : public ModuleCache::Module {
  public:
-  AndroidModule(unwindstack::MapInfo* map_info)
+  NonElfModule(unwindstack::MapInfo* map_info)
       : start_(map_info->start),
-        size_(map_info->end - map_info->start),
-        build_id_(EncodeBuildID(map_info->GetBuildID())),
-        debug_basename_(CreateDebugBasename(map_info->name)) {}
-  ~AndroidModule() override = default;
+        size_(map_info->end - start_),
+        map_info_name_(map_info->name) {}
+  ~NonElfModule() override = default;
 
   uintptr_t GetBaseAddress() const override { return start_; }
 
-  std::string GetId() const override { return build_id_; }
+  std::string GetId() const override { return std::string(); }
 
-  FilePath GetDebugBasename() const override { return debug_basename_; }
+  FilePath GetDebugBasename() const override {
+    return FilePath(map_info_name_);
+  }
 
   // Gets the size of the module.
   size_t GetSize() const override { return size_; }
@@ -80,10 +53,10 @@ class AndroidModule : 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 build_id_;
-  const FilePath debug_basename_;
+  const std::string map_info_name_;
 };
 
 std::unique_ptr<unwindstack::Regs> CreateFromRegisterContext(
@@ -104,10 +77,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(uint32_t));
+         unwindstack::ARM_REG_LAST * sizeof(uintptr_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(uint32_t));
+         unwindstack::ARM64_REG_LAST * sizeof(uintptr_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)
@@ -244,12 +217,29 @@ UnwindResult NativeUnwinderAndroid::TryUnwind(RegisterContext* thread_context,
 void NativeUnwinderAndroid::AddInitialModulesFromMaps(
     const unwindstack::Maps& memory_regions_map,
     ModuleCache* module_cache) {
-  for (const auto& region : memory_regions_map) {
-    // Only add executable regions.
+  // 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) {
     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<AndroidModule>(region.get()));
+        std::make_unique<NonElfModule>(region.get()));
   }
 }
 
@@ -264,7 +254,7 @@ void NativeUnwinderAndroid::EmitDexFrame(uintptr_t dex_pc,
     // AddInitialModules().
     unwindstack::MapInfo* map_info = memory_regions_map_->Find(dex_pc);
     if (map_info) {
-      auto new_module = std::make_unique<AndroidModule>(map_info);
+      auto new_module = std::make_unique<NonElfModule>(map_info);
       module = new_module.get();
       module_cache->AddCustomNativeModule(std::move(new_module));
     }

base/profiler/native_unwinder_android_unittest.cc

@@ -6,9 +6,12 @@
 
 #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"
@@ -20,6 +23,7 @@
 #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"
@@ -30,6 +34,11 @@ 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,
@@ -387,25 +396,8 @@ TEST(NativeUnwinderAndroidTest, UnwindStackMemoryTest) {
   check_read_succeeds(end - 1, 1);
 }
 
-// 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) {
+// Checks the debug basename is the whole name for a non-ELF module.
+TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForNonElf) {
   unwindstack::Maps maps;
 
   AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "[foo / bar]", {0xAA},
@@ -420,37 +412,48 @@ TEST(NativeUnwinderAndroidTest, ModuleDebugBasenameForNonPath) {
   EXPECT_EQ("[foo / bar]", modules[0]->GetDebugBasename().value());
 }
 
-// Checks that the specified build id is returned.
-TEST(NativeUnwinderAndroidTest, ModuleId) {
+// Checks that modules are only created for executable memory regions.
+TEST(NativeUnwinderAndroidTest, ModulesCreatedOnlyForExecutableRegions) {
   unwindstack::Maps maps;
-
-  AddMapInfo(0x1000u, 0x2000u, 0u, PROT_READ | PROT_EXEC, "/lib/foo.so",
-             {0x12, 0x34, 0x56, 0x78, 0x90, 0xAB, 0xCD, 0xEF}, 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);
 
   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(1u, modules.size());
-  // The id should have a '0' age field appended.
-  EXPECT_EQ("1234567890ABCDEF0", modules[0]->GetId());
+  ASSERT_EQ(2u, modules.size());
+  EXPECT_EQ(0x1000u, modules[0]->GetBaseAddress());
+  EXPECT_EQ(0x3000u, modules[1]->GetBaseAddress());
 }
 
-// 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);
-
+// Checks that module address ranges don't overlap.
+TEST(NativeUnwinderAndroidTest, NonOverlappingModules) {
   ModuleCache module_cache;
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(maps, &module_cache);
+  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
+  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();
+      });
 
-  ASSERT_EQ(1u, modules.size());
-  EXPECT_EQ(std::string(), modules[0]->GetId());
+  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));
 }
 
 // ModuleCache::GetModuleForAddress() is not implemented for 64-bit arm.
@@ -463,8 +466,9 @@ TEST(NativeUnwinderAndroidTest, EmptyModuleId) {
 // state created by the ModuleCache. Checks the module for a system library.
 TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
   ModuleCache unwinder_module_cache;
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(
-      *NativeUnwinderAndroid::CreateMaps(), &unwinder_module_cache);
+  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(*maps,
+                                                   &unwinder_module_cache);
 
   const uintptr_t c_library_function_address =
       reinterpret_cast<uintptr_t>(&printf);
@@ -486,11 +490,12 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
       reference_module_cache.GetModuleForAddress(c_library_function_address);
   ASSERT_NE(nullptr, reference_module);
 
-  // TODO(https://crbug.com/1004855): Fix base address and size discrepancies
-  // and add checks.
+  EXPECT_EQ(reference_module->GetBaseAddress(),
+            unwinder_module->GetBaseAddress());
   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.
@@ -504,8 +509,9 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_SystemLibrary) {
 // library.
 TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_ChromeLibrary) {
   ModuleCache unwinder_module_cache;
-  NativeUnwinderAndroid::AddInitialModulesFromMaps(
-      *NativeUnwinderAndroid::CreateMaps(), &unwinder_module_cache);
+  std::unique_ptr<unwindstack::Maps> maps = NativeUnwinderAndroid::CreateMaps();
+  NativeUnwinderAndroid::AddInitialModulesFromMaps(*maps,
+                                                   &unwinder_module_cache);
 
   const uintptr_t chrome_function_address =
       reinterpret_cast<uintptr_t>(&CaptureScenario);
@@ -533,7 +539,7 @@ TEST(NativeUnwinderAndroidTest, MAYBE_ModuleState_ChromeLibrary) {
   EXPECT_EQ(reference_module->GetId(), unwinder_module->GetId());
   EXPECT_EQ(reference_module->GetDebugBasename(),
             unwinder_module->GetDebugBasename());
-  // TODO(https://crbug.com/1004855): Fix size discrepancy and add check.
+  EXPECT_EQ(unwinder_module->GetSize(), reference_module->GetSize());
 }
 
 }  // namespace base