B32239 clang-tidy should not warn about array to pointer decay on system macros

[brenoguim]

Summary

Adding a new precondition to avoid warning about array to pointer decay on system macros.

If the ImplicitCastExpr match is casting a "system header symbol" from array to pointer happens inside a system header macro, clang-tidy should not trigger a violation.
I'm calling a "system header symbol" any symbol defined in a system header, or a PredefinedExpr type (like FILE).

Since this is my first submission, there are a few things I couldn't quite figure out:
1 - There is automatic support to switching warn about violations on system header. Maybe I should reuse some of this code?
2 - This check will be supressed even if -system-headers is passed on the command line. I believe, if I reuse the code mentioned in 1, this comes for free. If not possible, I can get the configuration and change my matcher.
I could not figure out how to change the matcher dynamically though. Is there any other way than doing:
if (SystemHeaders) {

Finder->addMatcher(. ...) ;

} else {

Finder->addMatcher(..., extraPrecondition())

}

[brenoguim]

Forgot to add cfe-commits...

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

50

Why not match on ImplicitCastExpr rather than Stmt? Then you can remove the explicit cast below.

55

Likewise here.

59–60

I think this is better-expressed with an isa check: if (isa<PredefinedExpr>(E->getSubExpr())) return true;

63–64

You can combine these into if (const auto *SDR = dyn_cast<>())

[brenoguim]

• Using the ImplicitCastExpr type on the AST_MATCHER directly to avoid explicit cast.
• Using isa<> instead of dyn_cast<> to just check for type
• Move variable declaration into "if" condition

[aaron.ballman]

LGTM!

[brenoguim]

Hi @aaron.ballman,

Thanks for the review!

I don't have rights to push this. Who should I contact to push the change in my behalf?

[aaron.ballman]

In D31130#706623, @brenoguim wrote:

Hi @aaron.ballman,

Thanks for the review!

I don't have rights to push this. Who should I contact to push the change in my behalf?

I'm happy to do so on your behalf (it'll happen later this afternoon unless someone else beats me to it).

[brenoguim]

I appreciate it!

[aaron.ballman]

I've commit in r298421.

[aaron.ballman]

This change was reverted in r298470. The use of the <assert.h> include is a problem because this is not a clang-supplied header file. Also, there's a (good) question about what array decay is happening in the assert(false) test case.

[alexfh]

In D31130#707197, @aaron.ballman wrote:

This change was reverted in r298470. The use of the <assert.h> include is a problem because this is not a clang-supplied header file. Also, there's a (good) question about what array decay is happening in the assert(false) test case.

I've made the test script add -nostdinc++ in r298501, which should help catching this kind of an issue earlier.

[brenoguim]

• Removed the "#include <assert.h>" which caused problems in environments without that header
• Included a directory with -isystem to simulate system headers
• Added a "macro.h" header with definitions of types, functions and macros to simulate the assert.h header and others.
• Added checks for behavior in some situations

Some of the checks follow the comment "Not Ok. Should it be ok?" to point the situations in which the check could be considered too strict for reporting such as violation. I would like to get feedback specially on them.

[mgehre]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

82

I would say that the check can ignore PredefinedExpr such as __PRETTY_FUNCTION__ by

unless(hasSourceExpression(predefinedExpr ())),

[brenoguim]

Never flag predefinedExpr() types.

[aaron.ballman]

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

60

I think it's reasonable to diagnose on this due to the decay.

86

Likewise here.

[fiesh]

Ping! Am I correct in that basically everything's done here and this has been lingering ever since? It would be great if the change could make it.

[lebedev.ri]

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

[fiesh]

In D31130#1711841, @lebedev.ri wrote:

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

What are the options in case the original author has abandonded this? Can anybody re-create the MR under a new license with the commit message fixed, or is that an insurmountable license issue?

[JonasToth]

In D31130#1758453, @fiesh wrote:

In D31130#1711841, @lebedev.ri wrote:

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

What are the options in case the original author has abandonded this? Can anybody re-create the MR under a new license with the commit message fixed, or is that an insurmountable license issue?

I guess we need specific permission from the author. maybe writing a mail to him/her would work? Then he/she can give permission (or decline it, which is unexpected i guess).

[fiesh]

I guess we need specific permission from the author. maybe writing a mail to him/her would work? Then he/she can give permission (or decline it, which is unexpected i guess).

I already wrote an email a couple weeks ago but haven't received a reply so far. Let's hope they come back to this thread eventually.

[ClausKlein]

I have problems with clang-tidy-9.0.0 on OS X and assert macro usage.
see https://github.com/isocpp/CppCoreGuidelines/issues/1589

In which clang version is the patch included?

[brenoguim]

Hey, I'm back. I actually kind of forgot about this.

Unfortunately I cannot to continue the patch soon. How do I grant you full rights on this patch? If just saying here is enough, there you go, anyone can take it and change it.
If there is more bureaucracy to it, just let me know.

I'll be happy to see any of the code making into the tool.

[brenoguim]

Oh, and if I remember correctly, the reason for this patch was to avoid exactly the github issue mentioned here. I'm embarrassed I didn't explain that anywhere :(

[fiesh]

@JonasToth could you please clarify if the original author's comment is sufficient grants of rights?

If so, from what I can tell if the commit message was adapted to reflect that this removes false positives with system macros like assert, this would be good to go?

[fiesh]

Ping everybody? This is a rather important issue I think since it makes clang-tidy not usable in a lot of cases, and the fix would be all done. Please let's get this merged!

[aaron.ballman]

In D31130#2220729, @fiesh wrote:

Ping everybody? This is a rather important issue I think since it makes clang-tidy not usable in a lot of cases, and the fix would be all done. Please let's get this merged!

Sorry for missing this before -- it's totally fine to pick up someone else's patch if the original author isn't able to work on it. IANAL, so I can't comment on the licensing question with any authority, but given that the original author says they're fine giving up full rights to their patch, that seems sufficient to me.

Given how long this review has sat idle for, I'd recommend starting a new review with a fully rebased patch, though.

[fiesh]

Continued in https://reviews.llvm.org/D88833

[brenoguim]

Nice! I'm glad to see the patch being revived.
Thanks for that :)

Em seg, 5 de out de 2020 10:48, fiesh via Phabricator <
[e-mail]> escreveu:

fiesh added a comment.

Continued in https://reviews.llvm.org/D88833

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D31130/new/

https://reviews.llvm.org/D31130

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

do {
     E = Parents[0].get<Expr>();
     if (!E)
       return false;
   } while (isa<ImplicitCastExpr>(E));
 
   return InnerMatcher.matches(*E, Finder, Builder);
 }
 
+AST_MATCHER(ImplicitCastExpr, isArrayToPointerDecay) {
+  return Node.getCastKind() == CK_ArrayToPointerDecay;
+}
+
+AST_MATCHER(ImplicitCastExpr, sysSymbolDecayInSysHeader) {
+  auto &SM = Finder->getASTContext().getSourceManager();
+  if (!SM.isInSystemMacro(Node.getLocStart())) {
+    return false;
+  }
+
+  if (const auto *SymbolDeclRef = dyn_cast<DeclRefExpr>(Node.getSubExpr())) {
+    const ValueDecl *SymbolDecl = SymbolDeclRef->getDecl();
+    if (SymbolDecl && SM.isInSystemHeader(SymbolDecl->getLocation()))
+      return true;
+  }
+  return false;
+}
+
 void ProBoundsArrayToPointerDecayCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   // The only allowed array to pointer decay
   // 1) just before array subscription
   // 2) inside a range-for over an array
   // 3) if it converts a string literal to a pointer
   Finder->addMatcher(
-      implicitCastExpr(unless(hasParent(arraySubscriptExpr())),
+      implicitCastExpr(isArrayToPointerDecay(),
+                       unless(hasParent(arraySubscriptExpr())),
                        unless(hasParentIgnoringImpCasts(explicitCastExpr())),
                        unless(isInsideOfRangeBeginEndStmt()),
-                       unless(hasSourceExpression(stringLiteral())))
+                       unless(hasSourceExpression(stringLiteral())),
+                       unless(hasSourceExpression(predefinedExpr())),
+                       unless(sysSymbolDecayInSysHeader()))
           .bind("cast"),
       this);
 }
 
 void ProBoundsArrayToPointerDecayCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *MatchedCast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
-  if (MatchedCast->getCastKind() != CK_ArrayToPointerDecay)
-    return;
 
   diag(MatchedCast->getExprLoc(), "do not implicitly decay an array into a "
                                   "pointer; consider using gsl::array_view or "
                                   "an explicit cast instead");
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

test/clang-tidy/Inputs/cppcoreguidelines-pro-bounds-array-to-pointer-decay/macro.h

+const char SYS_STRING[2]    = "s";
+const int  SYS_INT_ARRAY[2] = {0, 0};
+
+void sys_fn_decay_str(const char *);
+void sys_fn_decay_int_array(const int *);
+
+#define sys_macro_with_pretty_function_string_decay sys_fn_decay_str(__PRETTY_FUNCTION__)
+#define sys_macro_with_sys_string_decay             sys_fn_decay_str(SYS_STRING)
+#define sys_macro_with_sys_int_array_decay          sys_fn_decay_int_array(SYS_INT_ARRAY)
+
+#define user_code_in_sys_macro(expr) expr;
+
+#define decay_to_char_pointer_in_macro(expr) sys_fn_decay_str(expr)
+#define decay_to_int_pointer_in_macro(expr)  sys_fn_decay_int_array(expr)

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-array-to-pointer-decay %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-array-to-pointer-decay %t -- -- -isystem%S/Inputs/cppcoreguidelines-pro-bounds-array-to-pointer-decay
+
 #include <stddef.h>
+#include <macro.h>
 
 namespace gsl {
 template <class T>
 class array_view {
 public:
   template <class U, size_t N>
   array_view(U (&arr)[N]);
 };
const char *g() {
   return "clang"; // OK, decay string literal to pointer
 }
 
 void f2(void *const *);
 void bug25362() {
   void *a[2];
   f2(static_cast<void *const*>(a)); // OK, explicit cast
 }
+
+void user_fn_decay_str(const char *);
+void user_fn_decay_int_array(const int *);
+void bug32239() {
+  sys_macro_with_pretty_function_string_decay; // Ok
+  sys_macro_with_sys_string_decay;             // Ok
+  sys_macro_with_sys_int_array_decay;          // Ok
+
+  sys_fn_decay_str(__PRETTY_FUNCTION__); // Ok
+
+  sys_fn_decay_str(SYS_STRING);          // Not Ok - should it be ok?
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  sys_fn_decay_int_array(SYS_INT_ARRAY); // Not Ok
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  user_code_in_sys_macro(sys_fn_decay_str(__PRETTY_FUNCTION__)); // Ok
+
+  user_code_in_sys_macro(sys_fn_decay_str(SYS_STRING));          // Not Ok - should it be ok?
+  // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  user_code_in_sys_macro(sys_fn_decay_int_array(SYS_INT_ARRAY)); // Not Ok
+  // CHECK-MESSAGES: :[[@LINE-1]]:49: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  const char UserString[1] = "";
+  decay_to_char_pointer_in_macro(UserString); // Not Ok
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  decay_to_char_pointer_in_macro(__PRETTY_FUNCTION__); // Ok
+
+  int a[5];
+  decay_to_int_pointer_in_macro(a); // Not Ok
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  user_fn_decay_str(__PRETTY_FUNCTION__); // Ok
+
+  user_fn_decay_str(SYS_STRING); // Not Ok - should it be ok?
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+
+  user_fn_decay_int_array(SYS_INT_ARRAY); // Not ok
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+}