[PATCH] Make caching of memory mappings in sanitizer_procmaps optional

Alexander Potapenko glider at google.com
Sat Mar 23 03:51:12 PDT 2013



================
Comment at: lib/sanitizer_common/sanitizer_procmaps.h:44
@@ -43,2 +43,3 @@
  public:
   MemoryMappingLayout();
+  explicit MemoryMappingLayout(bool use_cache);
----------------
Pls add a comment that the default ctor makes MemoryMappingLayout use the cache.
OTOH I don't think we really need the default constructor - maybe it's better to always state whether we use the cache or no (Kostya, Alexey, WDYT?)

================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:347
@@ -337,3 +346,3 @@
   }
   // internal_write(2, proc_self_maps_.data, proc_self_maps_.len);
   Reset();
----------------
Good time to delete this line.

================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:343
@@ -334,2 +342,3 @@
   if (proc_self_maps_.mmaped_size == 0) {
+    CHECK(use_cache);
     LoadFromCache();
----------------
I think the actual invariant we want to check for is that proc_self_maps_.mmaped_size is greater than zero in the case use_cache is false.
Otherwise if ReadFileToBuffer fails you'll get a strange error message that use_cache is false (which should be correct if you're running LSan)
I suggest the following:

if use_cache is true {
  if mmaped_size == 0 (i.e. ReadFileToBuffer failed) {
    LoadFromCache()
    CHECK_GT(...)
  }
} else {
  CHECK(mmaped_size > 0)
}

(uh, this sucks - why can't I use indentation in comments?)


http://llvm-reviews.chandlerc.com/D569



More information about the llvm-commits mailing list