[clang-tidy] Fix for bugprone-sizeof-expression PR57167

[chrish_ericsson_atx]

Summary

This addresses a change in behavior of the bugprone-sizeof-expression
checker after upstream commit 15f3cd6bfc6, which cleaned up
ElaboratedType sugaring in the AST. This restores (mostly) the
beahvior of the checker prior to that commit, which may or may not have
been consistent with the intent of the checker, but at least gave a
tolerable level of what users would consider false positives.

Bug: https://github.com/llvm/llvm-project/issues/57167

[mizvekov]

LGTM once minor nits are fixed. I am not opposed to getting this back to where it was, we can take a more careful look later at what this checker should be doing instead.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c

63

Missing newline here.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

Is this change really desirable, or should we put a FIXME here?

[njames93]

Please can you run git clang-format over the patch to keep the pre-merge bot happy.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

Not warning on these cases seems like a pretty big red flag, especially the MyStruct *.

[chrish_ericsson_atx]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

Thank you for your comment! I'm not sure I fully agree that this is a red flag. I'm inclined to think that whether or not there should be a warning on MyStruct * or PMyStruct is a pretty subjective. These are both very common idioms, and are meaningful. I do appreciate the perspective that MyStruct * is just one character different from MyStruct, and as such, it might be a typo to ask for sizeof the pointer, when you really wanted sizeof the struct. But the counter-argument (accidentally asking for sizeof the struct when you really wanted the pointer size) is just as valid-- and the checker obviously cannot warn in that case.

I am perfectly fine with kicking the can down the road a bit by replacing the discarded // CHECK-MESSAGES directive with a // FIXME, as @mizvekov suggested. I expect that when someone circles back to really deeply reconsider the semantics of this checker, there will be a number of changes (other existing warnings dropped, warnings for new and missed cases added, much better sync between C and C++, as well as intentional consideration of things like typeof (in it's various forms) and decltype, rather than the haphazard coarse-grain matching that seems to be going on now.

[mizvekov]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

I agree with this patch only in so far that:

• This is effectively a partial revert of the changes made in https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
• The checker was pretty much buggy to begin with.
• That change significantly increased the amount of patterns we would accept, but at the same time the existing tests did not pick that up and this was not carefully considered.
• It seems unreasonable that there is effectively no way to shut this warning up per site, except by a NOLINT directive.

If the amount of false positives is so high now that this check is unusable, then this is just a question of reverting to a less broken state temporarily.

But otherwise we can't leave it in the reverted state either. Without that change to use the canonical type or something similar, there is no reason to suppose any of these test cases would work at all as clang evolves and we improve the quality of implementation wrt type sugar retention.

[chrish_ericsson_atx]

Added missing newline and added // FIXME comments, per reviewer comments.

[chrish_ericsson_atx]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

@mizvekov, I agree with your reasoning for saying "we can't leave it in the reverted state either". But I'm not sure how to ensure that this gets the needed attention. Should we just file a separate PR on github to track the needed refactoring? I do not believe I'll have the bandwidth to look at this in the next few months.

In the meantime, assuming the bots are happy with patchset 2, I'll land this as-is later today.

Thank you very much for your feedback!

[mizvekov]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

Oh please wait for others to review, I don't think landing today is reasonable!

I am not really a stakeholder for this checker except for that original change.

I would advise for you to wait for another more responsible reviewer to accept as well before merging, unless this gets stale for a long time and no one else seems to be interested.

[chrish_ericsson_atx]

@mizvekov : Sounds fair. I had taken your acceptance of the change as a green light. :) TBH, the acceptance came much faster than I'd expected-- even though I believe this to be a trivial and low-risk change, I expected it to sit for at least several days. I'll plan to wait a few more days before landing it.

[chrish_ericsson_atx]

Corrected git-clang-format issue.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c

43

Based on the documentation, I would expect this one to be diagnosed.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

Thank you for your comment! I'm not sure I fully agree that this is a red flag. I'm inclined to think that whether or not there should be a warning on MyStruct * or PMyStruct is a pretty subjective. These are both very common idioms, and are meaningful. I do appreciate the perspective that MyStruct * is just one character different from MyStruct, and as such, it might be a typo to ask for sizeof the pointer, when you really wanted sizeof the struct. But the counter-argument (accidentally asking for sizeof the struct when you really wanted the pointer size) is just as valid-- and the checker obviously cannot warn in that case.

I agree with @njames93 that this is a red flag. That check behavior is explicitly documented: https://releases.llvm.org/13.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-sizeof-expression.html#suspicious-usage-of-sizeof-a so this change is introducing a different kind of regression.

(However, there's no option for disabling that specific situation and I think there probably should be one, eventually.)

[mizvekov]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

This revert is almost entirely knocking out this PointerToStructType matcher, which matches sizeof of any record which is not written as an address-of operator expression.

I think with this revert and after the elaboratedType patch changes, the only case that should still trigger it should be a type substitution for a template argument written as a pointer to Record, because that should canonicalize the pointee without adding any other sugar on it.

It won't match a pointer-to-sugar-to-record anymore at all, as it happened before the ElaborateType patch, but before that patch, there was an additional case that would match: A pointer to a struct written without any elaboration, because we would not put any sugar on top of the RecordType.

The documentation does mention the Suspicious usage of ‘sizeof(A*)’ case, but it only gives as an example the sizeof-address-of-expression case, even though that is handled separately in the implementation here and not affected.

So I guess that might explain the contention here about the seriousness of this regression.

[aaron.ballman]

LGTM aside from removing some comments now that we figured out what's going on. Please hold off on landing for a day or two in case @njames93 has other opinions though.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–237

236–238

The documentation does mention the Suspicious usage of ‘sizeof(A*)’ case, but it only gives as an example the sizeof-address-of-expression case, even though that is handled separately in the implementation here and not affected.

So I guess that might explain the contention here about the seriousness of this regression.

Yeah, those docs only show the expression case and not the type case. That's an interesting point! It also says These cases may occur because of explicit cast or implicit conversion. which sure implies that this is an expression check rather than a type check.

I dug through the history to find the original review for this feature to see if this was discussed. That review is: https://reviews.llvm.org/D19014 I didn't spot any indication this was meant to diagnose sizeof(some_type).

So I guess I'm wrong on this being a red flag!

[chrish_ericsson_atx]

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

236–238

I understand the concerns. And I agree with @mizvekov's rationale that the documentation doesn't explicitly cover the two pre-existing cases that this change affects.

To further support that, the documentation describing diags for sizeof(A*) were introduced when the checker was introduced in https://reviews.llvm.org/D19014 in 2016. But the two test cases that are affected by my change were introduced 3 years later in https://reviews.llvm.org/D61260, and documentation was not updated as part of that later change.

That later change by @baloghadamsoftware seems to have been intended to broaden the checker to explicitly catch attempts to take sizeof struct-pointer or values of that type -- and my change largely reverts that behavior, but doesn't affect the original behavior of the checker (as documented). Looking at the review comments, I don't get a sense that the false positive rate for sizeof(A*) was considered carefully, and in particular, there was no discussion of targets with multiple address spaces and different size pointers, respectively, where one would expect to see sizeof(A*) done rather commonly, and clearly intentionally.

I reached out separately to Ádám for guidance on this change, and he's included on this review. I'll wait a bit to give him an opportunity to comment here.

[chrish_ericsson_atx]

Removed // FIXME comments, per @aaron.ballman's feedback.

clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp

const auto ArrayCastExpr = expr(anyOf(
       binaryOperator(hasEitherOperand(ArrayExpr)),
       castExpr(hasSourceExpression(ArrayExpr))));
   const auto PointerToArrayExpr = ignoringParenImpCasts(
       hasType(hasCanonicalType(pointerType(pointee(arrayType())))));
 
   const auto StructAddrOfExpr = unaryOperator(
       hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
                                 hasType(hasCanonicalType(recordType())))));
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-      pointerType(pointee(hasCanonicalType(recordType()))));
+  const auto PointerToStructType =
+      hasUnqualifiedDesugaredType(pointerType(pointee(recordType())));
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
       hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr())));
 
   const auto ArrayOfPointersExpr = ignoringParenImpCasts(
       hasType(hasCanonicalType(arrayType(hasElementType(pointerType()))
                                    .bind("type-of-array-of-pointers"))));
   const auto ArrayOfSamePointersExpr =
       ignoringParenImpCasts(hasType(hasCanonicalType(

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c

+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+    Array10 arr;
+    Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+    Array10 arr;
+    Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof(&S);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof(&S));
+  sum += sizeof(&TS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof(&TS));
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(&A10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct &rS = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

int Test5() {
   sum += sizeof(kGlocalMyStructPtr->ptr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
 
   sum += sizeof(&kGlocalMyStruct);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&S);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&A10);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(A10) / sizeof(PtrArray[0]);