[Diagnostics] Add -Wsizeof-array-div

[xbolva00]

Summary

Clang version of https://www.viva64.com/en/examples/v706/

[xbolva00]

Addressed comments by @rsmith

[xbolva00]

lib/Sema/SemaExpr.cpp

9169 ↗

(On Diff #219175)

If we dont skip isDependentType, compiler crashes later
Unknown builtin type!
UNREACHABLE executed at /home/xbolva00/LLVM/llvm/tools/clang/lib/AST/ASTContext.cpp:1855!
Stack dump:
0. Program arguments: /home/xbolva00/LLVM/build/bin/clang -cc1 -internal-isystem /home/xbolva00/LLVM/build/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -verify -std=c++11 /home/xbolva00/LLVM/llvm/tools/clang/test/SemaTemplate/instantiate-init.cpp

Even if skip hasSimilarType call, the warning message is unfriendly like "element type is "int", not "<dependent type>"

Ok to skip?

9174 ↗

(On Diff #219175)

fails to build with const Expr *, so workaround.

test/Sema/div-sizeof-array.cpp

16 ↗

(On Diff #219175)

We could warn here, but the code is working fine iff sizeof(unsigned long long) == sizeof(int *).

Maybe just ignore this case for now?

[rsmith]

lib/Sema/SemaExpr.cpp

9169 ↗

(On Diff #219175)

Yes, we can't compute the size of a dependent type. You should also check this for ArrayElemTy.

9170 ↗

(On Diff #219175)

You don't need this check if you're also checking sizes: similar types always have the same size.

9171 ↗

(On Diff #219175)

And we know that these types are complete or dependent because we already checked the formation of a sizeof expression on them? OK.

9174 ↗

(On Diff #219175)

Do not include pretty-printed expressions in diagnostics like this. If the LHS is a DeclRefExpr, you can print its name here, but otherwise you should just underline the expression rather than printing it out.

[xbolva00]

Addressed review notes

[xbolva00]

lib/Sema/SemaExpr.cpp

9174 ↗

(On Diff #219175)

Ok, done.

Implemented a bit different than suggested, maybe ok too?

[rsmith]

Just one thing I missed before, otherwise this looks good. Thanks!

lib/Sema/SemaExpr.cpp

9171 ↗

(On Diff #219591)

Use Context.getAsArrayType here to properly deal with type sugar.

[xbolva00]

• use S.Context.getAsArrayType

[xbolva00]

Thanks for review/advices!

[thakis]

Did you collect metrics on number of true/false positives of this warning?

[MaskRay]

In D67287#1665215, @xbolva00 wrote:

Thanks for review/advices!

It seems you committed in a hurry. This adds a new enabled-by-default warning which can break people's builds if they have -Werror. I don't know who will be broken but I think you probably needs a thumbs-up before committing.

cfe/trunk/test/Sema/div-sizeof-array.cpp

1

For enabled-by-default warnings, I guess it may be nice to have another RUN line without -Wsizeof-array-div.

[xbolva00]

Can sou show me a false positive case?

[xbolva00]

This adds a new enabled-by-default warning which can break people's builds if they have -Werror.

Sorry, but why a LLVM/Clang contributor should care if user’s -Werror builds may break just because a true positive?

If it breaks a build and finds a bug, then great, no?

[xbolva00]

gcc devs going to implement it too (inspired by this patch ;-))

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91741

[gribozavr]

FWIW, we did find a false positive with this warning.

https://github.com/bluerise/openbsd/blob/664a06af7e423ff06d46383c669d33c7bea7d234/lib/libutil/bcrypt_pbkdf.c#L81

uint32_t cdata[BCRYPT_WORDS];
...
blf_enc(&state, cdata, sizeof(cdata) / sizeof(uint64_t));

Looks like a bug indeed, until we look at the documentation for blf_enc, https://man.openbsd.org/blowfish, which says that the function indeed accepts 64-bit-sized blocks. However, for unclear reasons, it accepts the buffer as u_int32_t*. So the callers must have a buffer of u_int32_t, but compute its size in uint64_ts.

So, is it a false positive? Yes. However, the code is indeed questionable.

[xbolva00]

Thanks, ugh, that’s weird. Yes, very questionable code. I would probably change it to

unsigned blockSize = sizeof(uint64_t);
sizeof(cdata) / blockSize;

and add a code comment that interface requires such blocks. Warning will go away and code readability will be better.

[JamesNagurne]

@xbolva00

As mentioned in an inline comment, the test only works if the sizeof(int) != sizeof(int *) and sizeof(unsigned long long) == sizeof(int)

The test then also runs clang -cc1 with the default target. This means the test asserts that all clang targets match the assertions above.

My team supports an embedded ARM default LLVM product, and this test is now failing on our end because int is 4 bytes, unsigned long long is 8 bytes, and int * is 4 bytes.
This test needs to either work for all targets (via some macro hackery) or the target needs to be made explicit, which I believe would be preferable because the warning itself is not reliant upon the target, only the type sizes.

For instance, adding -triple x86_64-unknown-linux-gnu fixes the test on our embedded ARM toolchain.

[xbolva00]

I added that triple yesterday, failing bot is green with it. Did you pull latest sources?

[JamesNagurne]

Ah! My apologies. I had not pulled anything new down, as our process tends to hard-stop until an issue is resolved (something I'm thinking needs to be a little more flexible).

I'll pull the fix down then, thanks!

[leonardchan]

Not sure if we're still mentioning false positives here, but if we have something like a multidimensional array of a specific type and we want to find the number of elements inside it, this warning would also appear:

int arr[][5] = {
  {1, 2, 3, 4, 5},
  {6, 7, 8, 9, 0},
};

void func() {
  unsigned num_elems = sizeof(arr) / sizeof(int);  // Warns here
  assert(num_elems == 10);
}

Although one could argue that sizeof(arr) / sizeof(**arr) would be better while not producing the warning.

[xbolva00]

Hm, yes, I will disable this warning if the element type is an array I think.

Or just wait and see how gcc devs will implement this and whether they will restrict it or not.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_address_of_reference_null_compare : Warning<
   InGroup<TautologicalUndefinedCompare>;
 def note_reference_is_return_value : Note<"%0 returns a reference">;
 
 def note_pointer_declared_here : Note<
   "pointer %0 declared here">;
 def warn_division_sizeof_ptr : Warning<
   "'%0' will return the size of the pointer, not the array itself">,
   InGroup<DiagGroup<"sizeof-pointer-div">>;
+def warn_division_sizeof_array : Warning<
+  "expression does not compute the number of elements in this array; element "
+  "type is %0, not %1">,
+  InGroup<DiagGroup<"sizeof-array-div">>;
 
 def note_function_warning_silence : Note<
     "prefix with the address-of operator to silence this warning">;
 def note_function_to_function_call : Note<
     "suffix with parentheses to turn this into a function call">;
 def warn_impcast_objective_c_literal_to_bool : Warning<
     "implicit boolean conversion of Objective-C object literal always "
     "evaluates to true">,

cfe/trunk/lib/Sema/SemaExpr.cpp

static void DiagnoseDivisionSizeofPointerOrArray(Sema &S, Expr *LHS, Expr *RHS,
   QualType LHSTy = LHSArg->getType();
   QualType RHSTy;
 
   if (RUE->isArgumentType())
     RHSTy = RUE->getArgumentType();
   else
     RHSTy = RUE->getArgumentExpr()->IgnoreParens()->getType();
 
-  if (!LHSTy->isPointerType() || RHSTy->isPointerType())
-    return;
-  if (LHSTy->getPointeeType().getCanonicalType().getUnqualifiedType() !=
-      RHSTy.getCanonicalType().getUnqualifiedType())
+  if (LHSTy->isPointerType() && !RHSTy->isPointerType()) {
+    if (!S.Context.hasSameUnqualifiedType(LHSTy->getPointeeType(), RHSTy))
       return;
 
     S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS << LHS->getSourceRange();
     if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
       if (const ValueDecl *LHSArgDecl = DRE->getDecl())
         S.Diag(LHSArgDecl->getLocation(), diag::note_pointer_declared_here)
             << LHSArgDecl;
     }
+  } else if (const auto *ArrayTy = S.Context.getAsArrayType(LHSTy)) {
+    QualType ArrayElemTy = ArrayTy->getElementType();
+    if (ArrayElemTy->isDependentType() || RHSTy->isDependentType() ||
+        S.Context.getTypeSize(ArrayElemTy) == S.Context.getTypeSize(RHSTy))
+      return;
+    S.Diag(Loc, diag::warn_division_sizeof_array)
+        << LHSArg->getSourceRange() << ArrayElemTy << RHSTy;
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
+      if (const ValueDecl *LHSArgDecl = DRE->getDecl())
+        S.Diag(LHSArgDecl->getLocation(), diag::note_array_declared_here)
+            << LHSArgDecl;
+    }
+  }
 }
 
 static void DiagnoseBadDivideOrRemainderValues(Sema& S, ExprResult &LHS,
                                                ExprResult &RHS,
                                                SourceLocation Loc, bool IsDiv) {
   // Check for division/remainder by zero.
   Expr::EvalResult RHSValue;
   if (!RHS.get()->isValueDependent() &&

cfe/trunk/test/Sema/div-sizeof-array.cpp

+// RUN: %clang_cc1 %s -verify -Wsizeof-array-div -fsyntax-only
+
+template <typename Ty, int N>
+int f(Ty (&Array)[N]) {
+  return sizeof(Array) / sizeof(Ty); // Should not warn
+}
+
+typedef int int32;
+
+void test(void) {
+  int arr[12];                // expected-note 2 {{array 'arr' declared here}}
+  unsigned long long arr2[4];
+  int *p = &arr[0];
+  int a1 = sizeof(arr) / sizeof(*arr);
+  int a2 = sizeof arr / sizeof p; // expected-warning {{expression does not compute the number of elements in this array; element type is 'int', not 'int *'}}
+  int a4 = sizeof arr2 / sizeof p;
+  int a5 = sizeof(arr) / sizeof(short); // expected-warning {{expression does not compute the number of elements in this array; element type is 'int', not 'short'}}
+  int a6 = sizeof(arr) / sizeof(int32);
+  int a7 = sizeof(arr) / sizeof(int);
+  int a9 = sizeof(arr) / sizeof(unsigned int);
+  const char arr3[2] = "A";
+  int a10 = sizeof(arr3) / sizeof(char);
+
+  int arr4[10][12];                         // expected-note 3 {{array 'arr4' declared here}}
+  int b1 = sizeof(arr4) / sizeof(arr2[12]); // expected-warning {{expression does not compute the number of elements in this array; element type is 'int [12]', not 'unsigned long long'}}
+  int b2 = sizeof(arr4) / sizeof(int *);    // expected-warning {{expression does not compute the number of elements in this array; element type is 'int [12]', not 'int *'}}
+  int b3 = sizeof(arr4) / sizeof(short *);  // expected-warning {{expression does not compute the number of elements in this array; element type is 'int [12]', not 'short *'}}
+}