Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit be788d89 authored by Christopher Ferris's avatar Christopher Ferris
Browse files

Allow multiple threads sharing a map to unwind. 

Add a mutex in MapInfo, and a mutex in Elf. Lock the creation of an Elf
file using the MapInfo mutex, and lock when calling Step, GetFunctionName,
or GetSoname since they can modify information in the object. It might
be beneficial to use a fine grained lock in the future.

Change the Maps object to contain a vector of MapInfo pointers rather
than the total objects. This avoids copying this data around.

Add a test to libbacktrace to verify that sharing a map while doing
unwinds in different threads works.

Add concurrency tests in libunwindstack to verify the locking works.

Add always inline to the RegsGetLocal arm and aarch64 functions. I had
a case where clang did not inline the code, so make sure this is specified.

Bug: 68813077

Test: New unit tests to cover the case. Passes all unit tests.
Test: Ran a monkey test while dumping bugreports and verified that
Test: no crashes in libunwind.
Test: Remove the locking and verified that all of the concurrenty tests fail.
Change-Id: I769e728c676f6bdae9e64ce4cdc03b6749beae03
parent f03f2a5c

libbacktrace/UnwindStackMap.cpp

+6 −6
Original line number Diff line number Diff line
@@ -44,15 +44,15 @@ bool UnwindStackMap::Build() { 
  }



  // Iterate through the maps and fill in the backtrace_map_t structure.

  for (auto& map_info : *stack_maps_) {

  for (auto* map_info : *stack_maps_) {

    backtrace_map_t map;

    map.start = map_info.start;

    map.end = map_info.end;

    map.offset = map_info.offset;

    map.start = map_info->start;

    map.end = map_info->end;

    map.offset = map_info->offset;

    // Set to -1 so that it is demand loaded.

    map.load_bias = static_cast<uintptr_t>(-1);

    map.flags = map_info.flags;

    map.name = map_info.name;

    map.flags = map_info->flags;

    map.name = map_info->name;



    maps_.push_back(map);

  }

libbacktrace/backtrace_test.cpp

+14 −43
Original line number Diff line number Diff line
@@ -77,6 +77,7 @@ struct thread_t { 


struct dump_thread_t {

  thread_t thread;

  BacktraceMap* map;

  Backtrace* backtrace;

  int32_t* now;

  int32_t done;
@@ -632,7 +633,7 @@ static void* ThreadDump(void* data) { 
  }



  // The status of the actual unwind will be checked elsewhere.

  dump->backtrace = Backtrace::Create(getpid(), dump->thread.tid);

  dump->backtrace = Backtrace::Create(getpid(), dump->thread.tid, dump->map);

  dump->backtrace->Unwind(0);



  android_atomic_acquire_store(1, &dump->done);
@@ -640,8 +641,8 @@ static void* ThreadDump(void* data) { 
  return nullptr;

}



TEST(libbacktrace, thread_multiple_dump) {

  // Dump NUM_THREADS simultaneously.

static void MultipleThreadDumpTest(bool share_map) {

  // Dump NUM_THREADS simultaneously using the same map.

  std::vector<thread_t> runners(NUM_THREADS);

  std::vector<dump_thread_t> dumpers(NUM_THREADS);
@@ -662,12 +663,17 @@ TEST(libbacktrace, thread_multiple_dump) { 


  // Start all of the dumpers at once, they will spin until they are signalled

  // to begin their dump run.

  std::unique_ptr<BacktraceMap> map;

  if (share_map) {

    map.reset(BacktraceMap::Create(getpid()));

  }

  int32_t dump_now = 0;

  for (size_t i = 0; i < NUM_THREADS; i++) {

    dumpers[i].thread.tid = runners[i].tid;

    dumpers[i].thread.state = 0;

    dumpers[i].done = 0;

    dumpers[i].now = &dump_now;

    dumpers[i].map = map.get();



    ASSERT_TRUE(pthread_create(&dumpers[i].thread.threadId, &attr, ThreadDump, &dumpers[i]) == 0);

  }
@@ -689,47 +695,12 @@ TEST(libbacktrace, thread_multiple_dump) { 
  }

}



TEST(libbacktrace, thread_multiple_dump_same_thread) {

  pthread_attr_t attr;

  pthread_attr_init(&attr);

  pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);

  thread_t runner;

  runner.tid = 0;

  runner.state = 0;

  ASSERT_TRUE(pthread_create(&runner.threadId, &attr, ThreadMaxRun, &runner) == 0);



  // Wait for tids to be set.

  ASSERT_TRUE(WaitForNonZero(&runner.state, 30));



  // Start all of the dumpers at once, they will spin until they are signalled

  // to begin their dump run.

  int32_t dump_now = 0;

  // Dump the same thread NUM_THREADS simultaneously.

  std::vector<dump_thread_t> dumpers(NUM_THREADS);

  for (size_t i = 0; i < NUM_THREADS; i++) {

    dumpers[i].thread.tid = runner.tid;

    dumpers[i].thread.state = 0;

    dumpers[i].done = 0;

    dumpers[i].now = &dump_now;



    ASSERT_TRUE(pthread_create(&dumpers[i].thread.threadId, &attr, ThreadDump, &dumpers[i]) == 0);

  }



  // Start all of the dumpers going at once.

  android_atomic_acquire_store(1, &dump_now);



  for (size_t i = 0; i < NUM_THREADS; i++) {

    ASSERT_TRUE(WaitForNonZero(&dumpers[i].done, 30));



    ASSERT_TRUE(dumpers[i].backtrace != nullptr);

    VerifyMaxDump(dumpers[i].backtrace);



    delete dumpers[i].backtrace;

    dumpers[i].backtrace = nullptr;

TEST(libbacktrace, thread_multiple_dump) {

  MultipleThreadDumpTest(false);

}



  // Tell the runner thread to exit its infinite loop.

  android_atomic_acquire_store(0, &runner.state);

TEST(libbacktrace, thread_multiple_dump_same_map) {

  MultipleThreadDumpTest(true);

}



// This test is for UnwindMaps that should share the same map cursor when

libunwindstack/Elf.cpp

+5 −0
Original line number Diff line number Diff line
@@ -18,6 +18,7 @@ 
#include <string.h>



#include <memory>

#include <mutex>

#include <string>



#define LOG_TAG "unwind"
@@ -87,6 +88,7 @@ void Elf::InitGnuDebugdata() { 
}



bool Elf::GetSoname(std::string* name) {

  std::lock_guard<std::mutex> guard(lock_);

  return valid_ && interface_->GetSoname(name);

}
@@ -95,6 +97,7 @@ uint64_t Elf::GetRelPc(uint64_t pc, const MapInfo* map_info) { 
}



bool Elf::GetFunctionName(uint64_t addr, std::string* name, uint64_t* func_offset) {

  std::lock_guard<std::mutex> guard(lock_);

  return valid_ && (interface_->GetFunctionName(addr, load_bias_, name, func_offset) ||

                    (gnu_debugdata_interface_ && gnu_debugdata_interface_->GetFunctionName(

                                                     addr, load_bias_, name, func_offset)));
@@ -119,6 +122,8 @@ bool Elf::Step(uint64_t rel_pc, uint64_t elf_offset, Regs* regs, Memory* process 
  }

  rel_pc -= load_bias_;



  // Lock during the step which can update information in the object.

  std::lock_guard<std::mutex> guard(lock_);

  return interface_->Step(rel_pc, regs, process_memory, finished) ||

         (gnu_debugdata_interface_ &&

          gnu_debugdata_interface_->Step(rel_pc, regs, process_memory, finished));

libunwindstack/MapInfo.cpp

+4 −0
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ 
#include <unistd.h>



#include <memory>

#include <mutex>

#include <string>



#include <unwindstack/Elf.h>
@@ -105,6 +106,9 @@ Memory* MapInfo::CreateMemory(const std::shared_ptr<Memory>& process_memory) { 
}



Elf* MapInfo::GetElf(const std::shared_ptr<Memory>& process_memory, bool init_gnu_debugdata) {

  // Make sure no other thread is trying to add the elf to this map.

  std::lock_guard<std::mutex> guard(mutex_);



  if (elf) {

    return elf;

  }

libunwindstack/Maps.cpp

+47 −45
Original line number Diff line number Diff line
@@ -44,7 +44,7 @@ MapInfo* Maps::Find(uint64_t pc) { 
  size_t last = maps_.size();

  while (first < last) {

    size_t index = (first + last) / 2;

    MapInfo* cur = &maps_[index];

    MapInfo* cur = maps_[index];

    if (pc >= cur->start && pc < cur->end) {

      return cur;

    } else if (pc < cur->start) {
@@ -57,22 +57,22 @@ MapInfo* Maps::Find(uint64_t pc) { 
}



// Assumes that line does not end in '\n'.

static bool InternalParseLine(const char* line, MapInfo* map_info) {

static MapInfo* InternalParseLine(const char* line) {

  // Do not use a sscanf implementation since it is not performant.



  // Example linux /proc/<pid>/maps lines:

  // 6f000000-6f01e000 rwxp 00000000 00:0c 16389419   /system/lib/libcomposer.so

  char* str;

  const char* old_str = line;

  map_info->start = strtoul(old_str, &str, 16);

  uint64_t start = strtoul(old_str, &str, 16);

  if (old_str == str || *str++ != '-') {

    return false;

    return nullptr;

  }



  old_str = str;

  map_info->end = strtoul(old_str, &str, 16);

  uint64_t end = strtoul(old_str, &str, 16);

  if (old_str == str || !std::isspace(*str++)) {

    return false;

    return nullptr;

  }



  while (std::isspace(*str)) {
@@ -81,82 +81,81 @@ static bool InternalParseLine(const char* line, MapInfo* map_info) { 


  // Parse permissions data.

  if (*str == '\0') {

    return false;

    return nullptr;

  }

  map_info->flags = 0;

  uint16_t flags = 0;

  if (*str == 'r') {

    map_info->flags |= PROT_READ;

    flags |= PROT_READ;

  } else if (*str != '-') {

    return false;

    return nullptr;

  }

  str++;

  if (*str == 'w') {

    map_info->flags |= PROT_WRITE;

    flags |= PROT_WRITE;

  } else if (*str != '-') {

    return false;

    return nullptr;

  }

  str++;

  if (*str == 'x') {

    map_info->flags |= PROT_EXEC;

    flags |= PROT_EXEC;

  } else if (*str != '-') {

    return false;

    return nullptr;

  }

  str++;

  if (*str != 'p' && *str != 's') {

    return false;

    return nullptr;

  }

  str++;



  if (!std::isspace(*str++)) {

    return false;

    return nullptr;

  }



  old_str = str;

  map_info->offset = strtoul(old_str, &str, 16);

  uint64_t offset = strtoul(old_str, &str, 16);

  if (old_str == str || !std::isspace(*str)) {

    return false;

    return nullptr;

  }



  // Ignore the 00:00 values.

  old_str = str;

  (void)strtoul(old_str, &str, 16);

  if (old_str == str || *str++ != ':') {

    return false;

    return nullptr;

  }

  if (std::isspace(*str)) {

    return false;

    return nullptr;

  }



  // Skip the inode.

  old_str = str;

  (void)strtoul(str, &str, 16);

  if (old_str == str || !std::isspace(*str++)) {

    return false;

    return nullptr;

  }



  // Skip decimal digit.

  old_str = str;

  (void)strtoul(old_str, &str, 10);

  if (old_str == str || (!std::isspace(*str) && *str != '\0')) {

    return false;

    return nullptr;

  }



  while (std::isspace(*str)) {

    str++;

  }

  if (*str == '\0') {

    map_info->name = str;

    return true;

    return new MapInfo(start, end, offset, flags, "");

  }



  // Save the name data.

  map_info->name = str;

  std::string name(str);



  // Mark a device map in /dev/ and not in /dev/ashmem/ specially.

  if (map_info->name.substr(0, 5) == "/dev/" && map_info->name.substr(5, 7) != "ashmem/") {

    map_info->flags |= MAPS_FLAGS_DEVICE_MAP;

  if (name.substr(0, 5) == "/dev/" && name.substr(5, 7) != "ashmem/") {

    flags |= MAPS_FLAGS_DEVICE_MAP;

  }

  return true;

  return new MapInfo(start, end, offset, flags, name);

}



bool Maps::Parse() {
@@ -187,8 +186,8 @@ bool Maps::Parse() { 
      }

      *newline = '\0';



      MapInfo map_info;

      if (!InternalParseLine(line, &map_info)) {

      MapInfo* map_info = InternalParseLine(line);

      if (map_info == nullptr) {

        return_value = false;

        break;

      }
@@ -205,8 +204,7 @@ bool Maps::Parse() { 


Maps::~Maps() {

  for (auto& map : maps_) {

    delete map.elf;

    map.elf = nullptr;

    delete map;

  }

}
@@ -222,8 +220,8 @@ bool BufferMaps::Parse() { 
      end_of_line++;

    }



    MapInfo map_info;

    if (!InternalParseLine(line.c_str(), &map_info)) {

    MapInfo* map_info = InternalParseLine(line.c_str());

    if (map_info == nullptr) {

      return false;

    }

    maps_.push_back(map_info);
@@ -252,24 +250,27 @@ bool OfflineMaps::Parse() { 


  std::vector<char> name;

  while (true) {

    MapInfo map_info;

    ssize_t bytes = TEMP_FAILURE_RETRY(read(fd, &map_info.start, sizeof(map_info.start)));

    uint64_t start;

    ssize_t bytes = TEMP_FAILURE_RETRY(read(fd, &start, sizeof(start)));

    if (bytes == 0) {

      break;

    }

    if (bytes == -1 || bytes != sizeof(map_info.start)) {

    if (bytes == -1 || bytes != sizeof(start)) {

      return false;

    }

    bytes = TEMP_FAILURE_RETRY(read(fd, &map_info.end, sizeof(map_info.end)));

    if (bytes == -1 || bytes != sizeof(map_info.end)) {

    uint64_t end;

    bytes = TEMP_FAILURE_RETRY(read(fd, &end, sizeof(end)));

    if (bytes == -1 || bytes != sizeof(end)) {

      return false;

    }

    bytes = TEMP_FAILURE_RETRY(read(fd, &map_info.offset, sizeof(map_info.offset)));

    if (bytes == -1 || bytes != sizeof(map_info.offset)) {

    uint64_t offset;

    bytes = TEMP_FAILURE_RETRY(read(fd, &offset, sizeof(offset)));

    if (bytes == -1 || bytes != sizeof(offset)) {

      return false;

    }

    bytes = TEMP_FAILURE_RETRY(read(fd, &map_info.flags, sizeof(map_info.flags)));

    if (bytes == -1 || bytes != sizeof(map_info.flags)) {

    uint16_t flags;

    bytes = TEMP_FAILURE_RETRY(read(fd, &flags, sizeof(flags)));

    if (bytes == -1 || bytes != sizeof(flags)) {

      return false;

    }

    uint16_t len;
@@ -283,9 +284,10 @@ bool OfflineMaps::Parse() { 
      if (bytes == -1 || bytes != len) {

        return false;

      }

      map_info.name = std::string(name.data(), len);

      maps_.push_back(new MapInfo(start, end, offset, flags, std::string(name.data(), len)));

    } else {

      maps_.push_back(new MapInfo(start, end, offset, flags, ""));

    }

    maps_.push_back(map_info);

  }

  return true;

}