[clang][Sema] removes -Wfree-nonheap-object reference param false positive

[cjdb]

Summary

Taking the address of a reference parameter might be valid, and without
CFA, false positives are going to be more trouble than they're worth.

This reverts commit 499571ea835daf786626a0db1e12f890b6cd8f8d.

[dblaikie]

Thanks for following up on this.

• Probably best to commit the revert of the workarounds as a separate follow-up commit, but I appreciate seeing them here to understand that this patch addresses them

clang/lib/Sema/SemaChecking.cpp

10729–10731

It doesn't look like the parameter case is tested, is it? And is it necessary? I think it'd be reasonable to warn on void f1(int i) { free(&i); } - so I think it's only the "is it a reference type" that's the important property.

clang/test/Sema/warn-free-nonheap-object.cpp

38–41

Is it relevant that these are members? Or could they be globals just as well? Might be simpler/more obvious if they were globals, I think.

43–46

Are these notably different from the non-member examples above? I assume not & so I'd probably skip these test cases.

64–65

I don't think this code is necessary - since clang warnings aren't interprocedural, there's no need to flesh out the example this far - you can have the function standalone elsewhere, that takes a reference parameter and tries to free it after taking its address. Without any caller.

[cjdb]

Oops, looks like I never submitted my replies!

clang/lib/Sema/SemaChecking.cpp

10729–10731

Right, but it's also reasonable to warn on this, which isn't a parameter.

int i = 0;
int& r = i;
std::free(&r);

However, I will take you up on making sure that parameters are tested.

clang/test/Sema/warn-free-nonheap-object.cpp

38–41

This tests that members aren't overlooked, which has a distinct code path to normal variables IIRC.

[dblaikie]

clang/lib/Sema/SemaChecking.cpp

10729–10731

Catching this case seems unlikely to be feasible in a compiler diagnostic - because differentiating the false positives and true positives would require more complicated analysis than is usually done.

eg: We shouldn't warn on this:

int *i = (int*)std::malloc(sizeof(int));
int &j = *i;
std::free(&j);

clang/test/Sema/warn-free-nonheap-object.cpp

13–16

I probably wouldn't bother testing the combinatorial expansion of all options (std versus global, rvalue versus lvalue) - if the features are treated orthogonally in the code (there's little chance that some particular combination would have a problem if each separately didn't have a problem).

& generally I don't think there's any need to have callers of these functions in any context (member function or otherwise - as mentioned further down) - clang warnings generally aren't interprocedural, so there's no real risk that checking the function alone would result in different results than checking it with a caller as well.

64–65

^ ping on this.

[cjdb]

responds to feedback

[dblaikie]

Could probably haggle over the tests some more, but might be diminishing returns at this point. Looks good!

Might suggest separating the code cleanup mlir changes from the warning change (committing the warning change first, then as a follow-up committing the mlir change), ideally. Not a huge deal either way - but the usual "separable changes should be separated".

[cjdb]

rebases to activate CI

clang/lib/Sema/SemaChecking.cpp

if (isa<FieldDecl, FunctionDecl, VarDecl>(D)) {
     return;
   }
 }
 
 void CheckFreeArgumentsAddressof(Sema &S, const std::string &CalleeName,
                                  const UnaryOperator *UnaryExpr) {
   if (const auto *Lvalue = dyn_cast<DeclRefExpr>(UnaryExpr->getSubExpr())) {
     const Decl *D = Lvalue->getDecl();
-    if (isa<VarDecl, FunctionDecl>(D))
+    if (isa<DeclaratorDecl>(D))
+      if (!dyn_cast<DeclaratorDecl>(D)->getType()->isReferenceType())
         return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, D);
   }
 
   if (const auto *Lvalue = dyn_cast<MemberExpr>(UnaryExpr->getSubExpr()))
     return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr,
                                       Lvalue->getMemberDecl());
 }
 
 void CheckFreeArgumentsPlus(Sema &S, const std::string &CalleeName,

clang/test/Sema/warn-free-nonheap-object.cpp

 // RUN: %clang_cc1 -Wfree-nonheap-object -std=c++11 -x c++ -fsyntax-only -verify %s
 
 extern "C" void free(void *) {}
 
 namespace std {
 using size_t = decltype(sizeof(0));
 void *malloc(size_t);
 void free(void *p);
 } // namespace std
 
 int GI;
 
+void free_reference(char &x) { ::free(&x); }
+void free_reference(char &&x) { ::free(&x); }
+void std_free_reference(char &x) { std::free(&x); }
+void std_free_reference(char &&x) { std::free(&x); }
+
 struct S {
-  operator char *() { return ptr; }
+  operator char *() { return ptr1; }
 
   void CFree() {
-    ::free(&ptr); // expected-warning {{attempt to call free on non-heap object 'ptr'}}
-    ::free(&I);   // expected-warning {{attempt to call free on non-heap object 'I'}}
-    ::free(ptr);
+    ::free(&ptr1); // expected-warning {{attempt to call free on non-heap object 'ptr1'}}
+    ::free(&I);    // expected-warning {{attempt to call free on non-heap object 'I'}}
+    ::free(ptr1);
+    free_reference(*ptr2);
+    free_reference(static_cast<char&&>(*ptr3));
   }
 
   void CXXFree() {
-    std::free(&ptr); // expected-warning {{attempt to call std::free on non-heap object 'ptr'}}
-    std::free(&I);   // expected-warning {{attempt to call std::free on non-heap object 'I'}}
-    std::free(ptr);
+    std::free(&ptr1); // expected-warning {{attempt to call std::free on non-heap object 'ptr1'}}
+    std::free(&I);    // expected-warning {{attempt to call std::free on non-heap object 'I'}}
+    std::free(ptr1);
+    std_free_reference(*ptr2);
+    std_free_reference(static_cast<char&&>(*ptr3));
   }
 
 private:
-  char *ptr = (char *)std::malloc(10);
+  char *ptr1 = (char *)std::malloc(10);
+  char *ptr2 = (char *)std::malloc(10);
+  char *ptr3 = (char *)std::malloc(10);
   static int I;
 };
 
 int S::I = 0;
 
 void test1() {
   {
     free(&GI); // expected-warning {{attempt to call free on non-heap object 'GI'}}
   }
   {
     static int SI = 0;
     free(&SI); // expected-warning {{attempt to call free on non-heap object 'SI'}}
   }
   {
     int I = 0;
     free(&I); // expected-warning {{attempt to call free on non-heap object 'I'}}
   }
   {
     int I = 0;
     int *P = &I;
     free(P);
   }
   {
     void *P = std::malloc(8);
     free(P); // FIXME diagnosing this would require control flow analysis.
   }
   {
     int A[] = {0, 1, 2, 3};
     free(A); // expected-warning {{attempt to call free on non-heap object 'A'}}
   }
   {
     int A[] = {0, 1, 2, 3};
     free(&A); // expected-warning {{attempt to call free on non-heap object 'A'}}
void test2() {
     int *P = &I;
     std::free(P); // FIXME diagnosing this would require control flow analysis.
   }
   {
     void *P = std::malloc(8);
     std::free(P);
   }
   {
+    char* P = (char *)std::malloc(2);
+    std_free_reference(*P);
+  }
+  {
+    char* P = (char *)std::malloc(2);
+    std_free_reference(static_cast<char&&>(*P));
+  }
+  {
     int A[] = {0, 1, 2, 3};
     std::free(A); // expected-warning {{attempt to call std::free on non-heap object 'A'}}
   }
   {
     int A[] = {0, 1, 2, 3};
     std::free(&A); // expected-warning {{attempt to call std::free on non-heap object 'A'}}
   }
   {