Bug 1484828 - Don't skip libraries with non-zero offsets. r=jld
authorMarkus Stange <mstange@themasta.com>
Tue, 02 Oct 2018 23:20:02 +0000
changeset 487695 9b246aa2b9cd23f627c701d8ac8ed9cb705f1a2b
parent 487694 5af47e5b67f0618aab9cd47b4483f2a32458be61
child 487696 8924d3afd1d3c7f03f884f4cfa1a705fba360e11
push id246
push userfmarier@mozilla.com
push dateSat, 13 Oct 2018 00:15:40 +0000
reviewersjld
bugs1484828
milestone64.0a1
Bug 1484828 - Don't skip libraries with non-zero offsets. r=jld I don't understand what's happening in this code and would appreciate some guidance. It's possible that there are two different meanings of the word "offset" that are clashing here. SharedLibrary::GetOffset() means firstMappingStart - baseAddress. I get a feeling that the "zero offset" check here cares more about the mapping offset phdr.p_offset, but I'm not sure. Anyway, the purpose of this patch is the following: Now that the previous patch sets SharedLibrary::mOffset to something non-zero, this check breaks unwinding for the affected libraries. With the check removed, unwinding seems to work mostly fine, as evidenced by this profile: https://perfht.ml/2MBKzON But removing the check might break something else that I'm not seeing. Depends on D3835 Differential Revision: https://phabricator.services.mozilla.com/D3836
tools/profiler/core/EHABIStackWalk.cpp
--- a/tools/profiler/core/EHABIStackWalk.cpp
+++ b/tools/profiler/core/EHABIStackWalk.cpp
@@ -625,23 +625,19 @@ void EHAddrSpace::Update() {
   if (space)
     return;
 
   SharedLibraryInfo info = SharedLibraryInfo::GetInfoForSelf();
   std::vector<EHTable> tables;
 
   for (size_t i = 0; i < info.GetSize(); ++i) {
     const SharedLibrary &lib = info.GetEntry(i);
-    if (lib.GetOffset() != 0)
-      // TODO: if it has a name, and we haven't seen a mapping of
-      // offset 0 for that file, try opening it and reading the
-      // headers instead.  The only thing I've seen so far that's
-      // linked so as to need that treatment is the dynamic linker
-      // itself.
-      continue;
+    // FIXME: This isn't correct if the start address isn't p_offset 0, because
+    // the start address will not point at the file header. But this is worked
+    // around by magic number checks in the EHTable constructor.
     EHTable tab(reinterpret_cast<const void *>(lib.GetStart()),
               lib.GetEnd() - lib.GetStart(), lib.GetNativeDebugPath());
     if (tab.isValid())
       tables.push_back(tab);
   }
   space = new EHAddrSpace(tables);
 
   if (!sCurrent.compareExchange(nullptr, space)) {