Add MemoryMappedSection struct for two-level memory map iteration

[fjricci]

Summary

This will allow sanitizer_procmaps on mac to expose section information.

Only scan global sections containing data in LSan on darwin

__DATA segments on Darwin contain a large number of separate sections,
many of which cannot actually contain pointers, and contain const values or
objc metadata. Not scanning sections which cannot contain pointers significantly
improves performance.

On a medium-sized (~4000 files) internal project, I saw a speedup of about 30%
in standalone LSan's execution time (30% improvement in the time spent running
LSan, not the total program time).

[fjricci]

Fix a typo

[alekseyshl]

lib/sanitizer_common/sanitizer_procmaps.h

67 ↗

(On Diff #106660)

Is it possible to drop has_sections and MemoryMappedSection, and add the current section info right here, under #if MAC? No other but Mac specific code will need to be changed.

If it turns out to be beneficial to have MemoryMappedSection defined, well, maybe define it inside the MemoryMappedSegment, under the same #if and call it just Segment?

That would lead to a platform specific function AddSegmentAddressRange(MemoryMappedSegment *segment, LoadedModule *module); and all the Mac specific stuff will be contained in _mac files.

What do you think? It's totally possible I miss some details precluding us from doing so.

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

67 ↗

(On Diff #106660)

Hmm, I'm not sure I completely understand what you mean. The reason why the MemoryMappedSection is a separate struct is because each MemoryMappedSegment will have multiple MemoryMappedSection's (at least on Darwin), so there isn't a 1:1 correspondence. The MemoryMappedSection is also quite a bit lighter-weight/smaller than using a full MemoryMappedSegment struct for each section.

I'm alright with moving Section inside of MemoryMappedSegment.

I thought about making the MemoryMappedSection Darwin-specific (and I'm still open to it), but that will cause issues if we ever end up with non-platform specific code that needs to iterate at the level of a section (something like tsan etc). I *think* that this shouldn't ever happen, because any code that can tell the difference between a section and a segment will probably be mac-specific code by definition.

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

67 ↗

(On Diff #106660)

Ahh, there's another piece I forgot about in terms of has_section. The original reason why I added it is that not all Darwin segments contain sections, and in the case where there are no sections, you need to use the address range for the segment itself. So even if we move things into darwin-specific ifdefs, we'll still need has_section.

[fjricci]

Address comments and remove MemoryMappedSection class

[fjricci]

Fix whitespace

[alekseyshl]

Looks good, thank you!

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #106723)

Move private under #if

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #106723)

Unfortunately, the linter complains if you don't have a blank line before private. It looks a bit odd to me to have:

#if SANITIZER_MAC

private:
  friend class MemoryMappingLayout;

What do you think?

[alekseyshl]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #106723)

It's fine with me, it's better than NOLINT comment or private: outside of the #if

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #106723)

Sounds good.

[MatzeB]

Various greendragon builds started to timeout when running unittests after this commit. Any idea what causes this? Should we revert the commit to bring the builders back?

[MatzeB]

Example: http://lab.llvm.org:8080/green/job/clang-stage2-configure-Rlto_check/17551/console

...
PASS: AddressSanitizer-x86_64-darwin :: TestCases/Darwin/dump_registers.cc (911 of 2593)
Build timed out (after 45 minutes). Marking the build as aborted.
Set build name.
New build name is 'r308348 (#17551)'
Variable with name 'BUILD_DISPLAY_NAME' already exists, current value: 'r308348 (#17551)', new value: 'r308348 (#17551)'
Build was aborted
Recording test results
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

[fjricci]

Interesting to me that this would cause timeouts, but yeah, I'll revert and look into it.

[fjricci]

Reverted as rL308394 and rL308395

[MatzeB]

In D35422#813830, @fjricci wrote:

Reverted as rL308394 and rL308395

Thank you!

[fjricci]

Fix 10.11 buildbots by masking dyld section addresses

This is already done for segment vmaddr's and needs to be done for sections as well

[fjricci]

Use blacklist instead of whitelist

[fjricci]

Remove accidental arcanist squash

[kcc]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #107548)

Folks, please avoid ifdefs like this.
I'd appreciate if you can refactor the code to get rid of this ifdef (e.g move stuff to a separate file)

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #107548)

This file already has quite a few OS-based #ifdefs in it (which is why I didn't think adding another would be problematic). Is it worth trying to refactor in a way that they all go away, or just this one in particular?

[kcc]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #107548)

already has quite a few OS-based #ifdefs

Not too many in fact.
And when there are N bad things in a file it's not always an excuse to add (N+1)-th

Is it worth trying to refactor in a way that they all go away

Yes, please. It's the same amount of work anyway.
E.g. declare a private object

MemoryMappingLayoutRep *rep

and implement MemoryMappingLayoutRep in OS-specific files.

[fjricci]

lib/sanitizer_common/sanitizer_procmaps.h

60 ↗

(On Diff #107548)

Makes sense. Will work on this. I wonder if we could get things to a point where we started enforcing lint rules against platform-specific ifdefs (unless some are actually unavoidable).

compiler-rt/trunk/lib/sanitizer_common/sanitizer_procmaps.h

 #endif  // SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_NETBSD
 
 // Memory protection masks.
 static const uptr kProtectionRead = 1;
 static const uptr kProtectionWrite = 2;
 static const uptr kProtectionExecute = 4;
 static const uptr kProtectionShared = 8;
 
-struct MemoryMappedSegment {
+class MemoryMappedSegment {
+ public:
   MemoryMappedSegment(char *buff = nullptr, uptr size = 0)
       : filename(buff), filename_size(size) {}
   ~MemoryMappedSegment() {}
 
   bool IsReadable() { return protection & kProtectionRead; }
   bool IsWritable() { return protection & kProtectionWrite; }
   bool IsExecutable() { return protection & kProtectionExecute; }
   bool IsShared() { return protection & kProtectionShared; }
 
   uptr start;
   uptr end;
   uptr offset;
   char *filename;  // owned by caller
   uptr filename_size;
   uptr protection;
   ModuleArch arch;
   u8 uuid[kModuleUUIDSize];
+
+#if SANITIZER_MAC
+
+ private:
+  friend class MemoryMappingLayout;
+
+  template <typename Section>
+  void NextSectionLoad(LoadedModule *module);
+  void AddAddressRanges(LoadedModule *module);
+
+  uptr nsects_;
+  char *current_load_cmd_addr_;
+  u32 lc_type_;
+  uptr base_virt_addr_;
+  uptr addr_mask_;
+#endif
 };
 
 class MemoryMappingLayout {
  public:
   explicit MemoryMappingLayout(bool cache_enabled);
   ~MemoryMappingLayout();
   bool Next(MemoryMappedSegment *segment);
   void Reset();

compiler-rt/trunk/lib/sanitizer_common/sanitizer_procmaps_mac.cc

 #ifndef CPU_SUBTYPE_ARM_V7K
 #define CPU_SUBTYPE_ARM_V7K   ((cpu_subtype_t)12)
 #endif
 #ifndef CPU_TYPE_ARM64
 #define CPU_TYPE_ARM64        (CPU_TYPE_ARM | CPU_ARCH_ABI64)
 #endif
 
 namespace __sanitizer {
+template <typename Section>
+void MemoryMappedSegment::NextSectionLoad(LoadedModule *module) {
+  const Section *sc = (const Section *)current_load_cmd_addr_;
+  current_load_cmd_addr_ += sizeof(Section);
+
+  uptr sec_start = (sc->addr & addr_mask_) + base_virt_addr_;
+  uptr sec_end = sec_start + sc->size;
+  module->addAddressRange(sec_start, sec_end, IsExecutable(), IsWritable());
+}
+
+void MemoryMappedSegment::AddAddressRanges(LoadedModule *module) {
+  if (!nsects_) {
+    module->addAddressRange(start, end, IsExecutable(), IsWritable());
+    return;
+  }
+
+  do {
+    if (lc_type_ == LC_SEGMENT) {
+      NextSectionLoad<struct section>(module);
+#ifdef MH_MAGIC_64
+    } else if (lc_type_ == LC_SEGMENT_64) {
+      NextSectionLoad<struct section_64>(module);
+#endif
+    }
+  } while (--nsects_);
+}
 
 MemoryMappingLayout::MemoryMappingLayout(bool cache_enabled) {
   Reset();
 }
 
 MemoryMappingLayout::~MemoryMappingLayout() {
 }
 

 // segment.
 // Note that the segment addresses are not necessarily sorted.
 template <u32 kLCSegment, typename SegmentCommand>
 bool MemoryMappingLayout::NextSegmentLoad(MemoryMappedSegment *segment) {
   const char *lc = current_load_cmd_addr_;
   current_load_cmd_addr_ += ((const load_command *)lc)->cmdsize;
   if (((const load_command *)lc)->cmd == kLCSegment) {
     const SegmentCommand* sc = (const SegmentCommand *)lc;
+    segment->current_load_cmd_addr_ = (char *)lc + sizeof(SegmentCommand);
+    segment->lc_type_ = kLCSegment;
+    segment->nsects_ = sc->nsects;
 
     if (current_image_ == kDyldImageIdx) {
+      segment->base_virt_addr_ = (uptr)get_dyld_hdr();
       // vmaddr is masked with 0xfffff because on macOS versions < 10.12,
       // it contains an absolute address rather than an offset for dyld.
       // To make matters even more complicated, this absolute address
       // isn't actually the absolute segment address, but the offset portion
       // of the address is accurate when combined with the dyld base address,
       // and the mask will give just this offset.
-      segment->start = (sc->vmaddr & 0xfffff) + (uptr)get_dyld_hdr();
-      segment->end = (sc->vmaddr & 0xfffff) + sc->vmsize + (uptr)get_dyld_hdr();
+      segment->addr_mask_ = 0xfffff;
     } else {
-      const sptr dlloff = _dyld_get_image_vmaddr_slide(current_image_);
-      segment->start = sc->vmaddr + dlloff;
-      segment->end = sc->vmaddr + sc->vmsize + dlloff;
-    }
+      segment->base_virt_addr_ =
+          (uptr)_dyld_get_image_vmaddr_slide(current_image_);
+      segment->addr_mask_ = ~0;
+    }
+    segment->start =
+        (sc->vmaddr & segment->addr_mask_) + segment->base_virt_addr_;
+    segment->end = segment->start + sc->vmsize;
 
     // Return the initial protection.
     segment->protection = sc->initprot;
     segment->offset =
         (current_filetype_ == /*MH_EXECUTE*/ 0x2) ? sc->vmaddr : sc->fileoff;
     if (segment->filename) {
       const char *src = (current_image_ == kDyldImageIdx)
                             ? kDyldPath
#endif
   return false;
 }
 
 void MemoryMappingLayout::DumpListOfModules(
     InternalMmapVector<LoadedModule> *modules) {
   Reset();
   InternalScopedString module_name(kMaxPathLength);
   MemoryMappedSegment segment(module_name.data(), kMaxPathLength);
-  for (uptr i = 0; Next(&segment); i++) {
+  while (Next(&segment)) {
     if (segment.filename[0] == '\0') continue;
     LoadedModule *cur_module = nullptr;
     if (!modules->empty() &&
         0 == internal_strcmp(segment.filename, modules->back().full_name())) {
       cur_module = &modules->back();
     } else {
       modules->push_back(LoadedModule());
       cur_module = &modules->back();
       cur_module->set(segment.filename, segment.start, segment.arch,
                       segment.uuid, current_instrumented_);
     }
-    cur_module->addAddressRange(segment.start, segment.end,
-                                segment.IsExecutable(), segment.IsWritable());
+    segment.AddAddressRanges(cur_module);
   }
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC