[PATCH] [Sanitizer RT] Put the module name string ownership in Symbolizer in order

[Timur Iskhodzhanov]

PTAL


================
Comment at: sanitizer_symbolizer.cc:153
@@ -134,3 +152,3 @@
   BlockingMutexLock l(&mu_);
-  return PlatformFindModuleNameAndOffsetForAddress(pc, module_name,
-                                                   module_address);
+  const char *internal_module_name = 0;
+  if (!PlatformFindModuleNameAndOffsetForAddress(pc, &internal_module_name,
----------------
samsonov wrote:
> nullptr
Done

================
Comment at: sanitizer_symbolizer.h:90
@@ -87,1 +89,3 @@
+  // module.  It is safe to store and compare them as pointers as long as the
+  // Symbolizer owning these strings is alive.
   bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
----------------
kcc wrote:
> Can you modify the comments (and logic) by removing "as long as .. "
> These names should be just live forever, independent of anything. 
Comment – updated.  What logic do I need to change now that the dtor is deleted?

================
Comment at: sanitizer_symbolizer.h:120
@@ +119,3 @@
+  // corresponding module might get unloaded later.  To prevent use-after-free
+  // race conditions, create Symbolizer-owned copies of the strings that we can
+  // safely return.
----------------
kcc wrote:
> Why race conditions? 
> This may happen w/o any threads, right? 
Theoretically – yes, but not with the current code.
Updated the comment.

================
Comment at: sanitizer_symbolizer.h:124
@@ +123,3 @@
+   public:
+    ModuleNameOwner() : storage_(1000), last_match_(nullptr) {}
+    const char *GetOwnedCopy(const char *str);
----------------
samsonov wrote:
> Please use a named constant.
Done

================
Comment at: sanitizer_symbolizer.h:125
@@ +124,3 @@
+    ModuleNameOwner() : storage_(1000), last_match_(nullptr) {}
+    const char *GetOwnedCopy(const char *str);
+
----------------
kcc wrote:
> add a comment explaining thread-(un) safety and which lock should be held. 
Done

================
Comment at: sanitizer_symbolizer.h:128
@@ +127,3 @@
+   private:
+    InternalMmapVector<char*> storage_;
+    char *last_match_;
----------------
samsonov wrote:
> const char *
yeah, done

================
Comment at: sanitizer_symbolizer.h:129
@@ +128,3 @@
+    InternalMmapVector<char*> storage_;
+    char *last_match_;
+  } module_names_;
----------------
samsonov wrote:
> Why is this not a const char * ?
Growth rings...  Fixed, thanks for catching!

http://reviews.llvm.org/D8666

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/