[PATCH] D31295: Add free_on_realloc_zero=true flag for compatibility with allocators which allow a realloc(p, 0) and don't free the pointer.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/asan/asan_allocator.cc:803
   if (size == 0) {
-    instance.Deallocate(p, 0, stack, FROM_MALLOC);
-    return nullptr;
+    if (flags()->free_on_realloc_zero) {
+      instance.Deallocate(p, 0, stack, FROM_MALLOC);
----------------
kcc wrote:
> maybe "free_and return_null_on_realloc_zero"? 
> 
> Os some other name with the opposite meaning (and default=false): 
>   allocator_returns_non_null_on_realloc_zero?
> 
> 
I phrased it in terms of the allocator, and explicitly mentioned is returns null new proposal for name (it's a bit long, but I can't shorten it and still mention all those points): `allocator_frees_and_returns_null_on_realloc_zero`


================
Comment at: lib/asan/asan_allocator.cc:806
+      return nullptr;
+    } else {
+      // Allocate a size of 1 if we shouldn't free() on Realloc to 0
----------------
alekseyshl wrote:
> No need in else block after return, less nesting is better.
Thanks. Done!


================
Comment at: test/asan/TestCases/realloc.cc:5
+// RUN: %env_asan_opts=free_on_realloc_zero=true %run %t 2>&1 | FileCheck %s
+// RUN: %env_asan_opts=free_on_realloc_zero=false %run %t 2>&1 | FileCheck %s -check-prefix=NO-FREE -allow-empty
+
----------------
alekseyshl wrote:
> alekseyshl wrote:
> > --allow-empty --check-prefix=NO-FREE
> > 
> > to match other tests
> Why do you need --allow-empty? Isn't it always supposed to print  "Allocated something on realloc(p, 0)"?
Was from a previous iteration of the test. Removed.


================
Comment at: test/asan/TestCases/realloc.cc:11
+int main(int argc, char **argv) {
+  void *p = malloc(argc);
+  p = realloc(p, 0);
----------------
alekseyshl wrote:
> Why argc and not the particular number of bytes?
It was originally to try to avoid clang optimizing away the memory allocations. Which doesn't make sense since I'm passing `-O0`. Doesn't hurt, though. Let me know if you'd rather have a constant anyway.


================
Comment at: test/asan/TestCases/realloc.cc:13
+  p = realloc(p, 0);
+  if (p)
+    // NO-FREE: Allocated something on realloc(p, 0)
----------------
alekseyshl wrote:
> if (p) {
> } else {
> }
Added braces.


https://reviews.llvm.org/D31295