[PATCH] D94640: adds more checks to -Wfree-nonheap-object

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: NoQ, Szelethus, rsmith.
aaron.ballman added a comment.
Herald added a subscriber: Charusso.

In D94640#2560647 <https://reviews.llvm.org/D94640#2560647>, @cjdb wrote:

> fixes block expressions
>
> @aaron.ballman I'm not sure I follow what you're after re lambdas. Could you please provide a test case that I can work with?

Sorry about missing your question -- this fell off my radar. I was thinking it'd be roughly the same as blocks, but I now think this'll create compile errors anyway: `free([]{ return nullptr; });` (where the lambda is accidentally not being called). So not nearly as important as I was thinking (sorry for that, but thank you for adding the support and test cases just the same!).

One worry I have is the number of effectively duplicated diagnostics we're now getting in conjunction with the static analyzer. It's not quite perfect overlap and so we can't get rid of the static analyzer check, but is there a long-term plan for how to resolve this?



================
Comment at: clang/lib/Sema/SemaChecking.cpp:10252
 namespace {
-void CheckFreeArgumentsOnLvalue(Sema &S, const std::string &CalleeName,
-                                const UnaryOperator *UnaryExpr,
-                                const VarDecl *Var) {
-  StorageClass Class = Var->getStorageClass();
-  if (Class == StorageClass::SC_Extern ||
-      Class == StorageClass::SC_PrivateExtern ||
-      Var->getType()->isReferenceType())
-    return;
-
-  S.Diag(UnaryExpr->getBeginLoc(), diag::warn_free_nonheap_object)
-      << CalleeName << Var;
-}
+constexpr int Placeholder = 0;
 
----------------
After seeing my suggested workaround in practice, I'm hopeful we can figure out how to get the diagnostics engine to not require us to jump through these hoops. I wouldn't block the patch over this approach, but it's not very obvious what's happening from reading the code either.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10342
+      static constexpr int NoPlaceholder = 1;
+      Diag(Arg->getBeginLoc(), diag::warn_free_nonheap_object)
+          << CalleeName << NoPlaceholder;
----------------
FWIW, this diagnostic will be awkward for lambdas because it mentions blocks explicitly. It's not critical thing given how hard it would be to hit the lambda case, but if you think of an easy way to solve it, I won't complain.


================
Comment at: clang/test/Analysis/free.c:66
 void t10 () {
-  free((void*)&t10); // expected-warning {{Argument to free() is the address of the function 't10', which is not memory allocated by malloc()}}
+  free((void*)&t10);
+  // expected-warning at -1{{Argument to free() is the address of the function 't10', which is not memory allocated by malloc()}}
----------------
This is another variant of the test that's slightly different (and only valid in C):
```
int *ptr(void);
void func(void) {
  free(ptr); // Oops, forgot to call ptr().
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94640/new/

https://reviews.llvm.org/D94640