[PATCH] [lsan] Implement __lsan_suppress_object().

[Sergey Matveev]

================
Comment at: include/sanitizer/lsan_interface.h:26
@@ -25,2 +25,3 @@
   void __lsan_enable();
-
+  // The heap object into which p points will be treated as a non-leak.
+  void __lsan_suppress_object(const void *p);
----------------
Alexey Samsonov wrote:
> What will happen for this:
> int *x = new int[10];
> __lsan_suppress_object(x + 5);
> 
> Shall we restrict arg value to "the heap object which p points to"?
If I understand correctly, HeapChecker currently accepts inside pointers in this case. We may want to implement it this way for the sake of backwards compatibility.

================
Comment at: include/sanitizer/lsan_interface.h:27
@@ -27,1 +26,3 @@
+  // The heap object into which p points will be treated as a non-leak.
+  void __lsan_suppress_object(const void *p);
 #ifdef __cplusplus
----------------
Kostya Serebryany wrote:
> why not ignore object? 
> Also, do we need unignore_object?
I don't like the fact that LSan often has two terms for the same concept - one home-grown and one inherited from Heap Checker. We already have "objects" vs "chunks", "live data" vs "non-leaks" vs "reachable memory", etc. I would prefer not to make it worse than it already is. I use the term "suppressed" for such objects throughout the code, so why introduce another term just for this function?

================
Comment at: include/sanitizer/lsan_interface.h:27
@@ -27,1 +26,3 @@
+  // The heap object into which p points will be treated as a non-leak.
+  void __lsan_suppress_object(const void *p);
 #ifdef __cplusplus
----------------
Sergey Matveev wrote:
> Kostya Serebryany wrote:
> > why not ignore object? 
> > Also, do we need unignore_object?
> I don't like the fact that LSan often has two terms for the same concept - one home-grown and one inherited from Heap Checker. We already have "objects" vs "chunks", "live data" vs "non-leaks" vs "reachable memory", etc. I would prefer not to make it worse than it already is. I use the term "suppressed" for such objects throughout the code, so why introduce another term just for this function?
HeapChecker has an UnignoreObject, but I really don't see a use case for it. (Actually, in HeapChecker you can't dealloc an ignored object without causing a warning, so that's probably why they added it). In Chromium they don't even have a macro defined for unignoring objects.

================
Comment at: lib/lsan/lsan_common.h:156
@@ -155,1 +155,3 @@
 
+enum SuppressObjectResult {
+  kOK,
----------------
Kostya Serebryany wrote:
> enum here looks weird.
> and then enum names too (esp kOk is awful)
What do you suggest?

================
Comment at: lib/sanitizer_common/sanitizer_allocator.h:347
@@ -346,3 +346,3 @@
 
-  static bool PointerIsMine(void *p) {
+  static bool PointerIsMine(const void *p) {
     return reinterpret_cast<uptr>(p) / kSpaceSize == kSpaceBeg / kSpaceSize;
----------------
Kostya Serebryany wrote:
> Why do you need const here? (I think you don't)
It's const in HeapChecker, so it must be const here too.

================
Comment at: lib/lsan/lsan_allocator.cc:217
@@ -201,3 +216,3 @@
   if (!__lsan::lsan_disabled) {
-    Report("Unmatched call to __lsan_enable().\n");
+    Report("LeakSanitizer: Unmatched call to __lsan_enable().\n");
     Die();
----------------
Alexey Samsonov wrote:
> Should you fix this message in ASan as well?
> Why don't you print stack trace for this location (and add a test case for it)?
Oops.
I should actually print stack trace for all fatal errors (especially those that happen under stoptheworld). Separate CL?

================
Comment at: lib/lsan/lsan_allocator.cc:194
@@ +193,3 @@
+
+SuppressObjectResult SuppressObjectLocked(const void *p) {
+  void *chunk = allocator.GetBlockBegin(p);
----------------
Kostya Serebryany wrote:
> you are duplicating code, aren't you? 
I call GetBlockBegin here, and access several metadata fields. I could move some of the logic into lsan_common, but then all of those operations would have to be exposed by *_allocator.cc. I think the current layout is a good compromise.


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