[clang] removes check against integral-to-pointer conversion...

[cjdb]

Summary

... unless it's a literal

D94640 was a bit too aggressive in its analysis, considering integers
representing valid addresses as invalid. This change rolls back some of
the check, so that only the most obvious case is still flagged.

Before:

cpp
free((void*)1000);   // literal converted to `void*`: warning good
free((void*)an_int); // `int` object converted to `void*`: warning might
                     //  be a false positive

After

cpp
free((void*)1000);   // literal converted to `void*`: warning good
free((void*)an_int); // doesn't warn

[lebedev.ri]

Patch is missing a description of the problem.

[cjdb]

updates commit message to explain what's going on and why the change

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

10320

We don't typically use top-level const on locals or params.

10326

I'm not 100% certain, but would IgnoreParenImpCasts() be sufficient here? (`IgnoreImplicitAsWritten() seems to be a bit special -- the only use of it I can find in tree is for rewritten binary operator expressions.)

clang/test/Analysis/free.cpp

221

I think it'd be useful to add tests for named casts as well as the C-style casts.

[cjdb]

applies comments

[cjdb]

clang/lib/Sema/SemaChecking.cpp

10320

This makes Christopher sad :(

10326

@rsmith would you mind weighing in here please? My original use-case for IgnoreImplicitAsWritten might be different to here and I don't have an answer for @aaron.ballman.

clang/test/Analysis/free.cpp

221

Great idea!

[cjdb]

s/IgnoreImplicitAsWritten/IgnoreParenImpCasts/ since the latter seems to work.

[cjdb]

clang/lib/Sema/SemaChecking.cpp

10326

I've tried out IgnoreParenImpCasts and it seems to work, but I'd appreciate an answer here for future patches please.

[thakis]

clang/lib/Sema/SemaChecking.cpp

10326

Can you rephrase your question? I'm not sure what exactly you want to know.

[cjdb]

clang/lib/Sema/SemaChecking.cpp

10326

Richard suggested I use IgnoreImplicitAsWritten in D94640, but it's apparently unnecessary in D97512. I'd like to better understand when to use IgnoreImplicitAsWritten. (BTW this is non-blocking, so if there are no other comments that need addressing, I'd like to press forward please.)

[aaron.ballman]

LGTM! I think we can resolve the question of which way to ignore implicit nodes and make any necessary changes post-commit.

clang/lib/Sema/SemaChecking.cpp

void CheckFreeArgumentsStackArray(Sema &S, const std::string &CalleeName,
   S.Diag(Lvalue->getBeginLoc(), diag::warn_free_nonheap_object)
       << CalleeName << 0 /*object: */ << Var;
 }
 
 void CheckFreeArgumentsCast(Sema &S, const std::string &CalleeName,
                             const CastExpr *Cast) {
   SmallString<128> SizeString;
   llvm::raw_svector_ostream OS(SizeString);
+
+  clang::CastKind Kind = Cast->getCastKind();
+  if (Kind == clang::CK_BitCast &&
+      !Cast->getSubExpr()->getType()->isFunctionPointerType())
+    return;
+  if (Kind == clang::CK_IntegralToPointer &&
+      !isa<IntegerLiteral>(
+          Cast->getSubExpr()->IgnoreImplicitAsWritten()->IgnoreParens()))
+    return;
+
   switch (Cast->getCastKind()) {
   case clang::CK_BitCast:
-    if (!Cast->getSubExpr()->getType()->isFunctionPointerType())
-      return;
-    LLVM_FALLTHROUGH;
   case clang::CK_IntegralToPointer:
   case clang::CK_FunctionToPointerDecay:
     OS << '\'';
     Cast->printPretty(OS, nullptr, S.getPrintingPolicy());
     OS << '\'';
     break;
   default:
     return;

clang/test/Analysis/free.c

 }
 
 int *iptr(void);
 void t17(void) {
   free(iptr); // Oops, forgot to call iptr().
   // expected-warning@-1{{Argument to free() is the address of the function 'iptr', which is not memory allocated by malloc()}}
   // expected-warning@-2{{attempt to call free on non-heap object 'iptr'}}
 }
+
+struct S {
+  const char* p;
+};
+
+void t18 (struct S s) {
+  free((void*)(unsigned long long)s.p); // no warning
+}

clang/test/Analysis/free.cpp

void t17a (char **x, int offset) {
   // Unknown value
   free(x[offset]); // no-warning
 }
 
 void t17b (char **x, int offset) {
   // Unknown value
   std::free(x[offset]); // no-warning
 }
+
+struct S {
+  const char* p;
+};
+
+void t18_C_style_C_style_free (S s) {
+  free((void*)(unsigned long long)s.p); // no warning
+}
+
+void t18_C_style_C_style_std_free (S s) {
+  std::free((void*)(unsigned long long)s.p); // no warning
+}
+
+void t18_C_style_reinterpret_free (S s) {
+  free((void*)reinterpret_cast<unsigned long long>(s.p)); // no warning
+}
+
+void t18_C_style_reinterpret_std_free (S s) {
+  std::free((void*)reinterpret_cast<unsigned long long>(s.p)); // no warning
+}
+
+void t18_reinterpret_C_style_free (S s) {
+  free(reinterpret_cast<void*>((unsigned long long)(s.p))); // no warning
+}
+
+void t18_reinterpret_C_style_std_free (S s) {
+  std::free(reinterpret_cast<void*>((unsigned long long)(s.p))); // no warning
+}
+
+void t18_reinterpret_reinterpret_free (S s) {
+  free(reinterpret_cast<void*>(reinterpret_cast<unsigned long long>(s.p))); // no warning
+}
+
+void t18_reinterpret_reinterpret_std_free (S s) {
+  std::free(reinterpret_cast<void*>(reinterpret_cast<unsigned long long>(s.p))); // no warning
+}