[Diagnostics] Implement -Wsizeof-pointer-div

[xbolva00]

Summary

void test(int *arr) {

int arr_len = sizeof(arr) / sizeof(*arr);  // warn, incorrect way to compute number of array elements

}

Enabled under -Wall (same behaviour as GCC)

[xbolva00]

test/Sema/div-sizeof-ptr.c

9 ↗

(On Diff #168539)

GCC warns also in this case, but it is weird...

[xbolva00]

• small code improvements

[thakis]

It looks like you ran clang-format on all of lib/Sema/SemaExpr.cpp and changed many lines that are irrelevant to your patch. Can you undo that, please?

[xbolva00]

• fixed formatting noise

[xbolva00]

• added DefaultIgnore

[xbolva00]

ping

[jfb]

Can you add tests with arrays?
The error message doesn't really say what to do instead. What's wrong? What's correct instead?
In C++17 and later we should suggest using std::size for some cases instead.

[xbolva00]

In D52949#1259947, @jfb wrote:

Can you add tests with arrays?

Yes :)

The error message doesn't really say what to do instead. What's wrong? What's correct instead?

What do you suggest? "division produces the incorrect number of array elements; pass the length of array as a function argument"?

In C++17 and later we should suggest using std::size for some cases instead.

Ok, good idea.

[xbolva00]

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

@rsmith?

[xbolva00]

Ping

[MTC]

In D52949#1268640, @xbolva00 wrote:

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

IMHO, a clang-tidy checker is more suitable than clang static analyzer to indicate to the developers that we should prefer std::size to sizeof(data)/sizeof(*data).

What I want to know most is whether we should emit a warning for the code below. I haven't been able to test the patch yet, but intuitively, this patch will emit a warning.

int foo(int *p) {
    int d = sizeof(p) / sizeof(char); // Should we emit a warning here?
    return d;
}

GCC keep silent about this case, and I agree with it, see https://gcc.godbolt.org/z/ZToFqq. What do you think about this case?

[xbolva00]

No warning for your case.

[xbolva00]

• Added MTC's example case to test file

[xbolva00]

Ping @MTC or @rsmith

[xbolva00]

Any ideas for better warning message? Except for the warning text, I see this patch as ready.

[xbolva00]

• New warning text, added another test line

[MTC]

Sorry for the long delay for this patch! The implementation is fine for me. However, I'm the newbie on clang diagnostics and have no further insight into this checker. @aaron.ballman may have more valuable insights into this checker.

test/Sema/div-sizeof-ptr.c

13 ↗

(On Diff #171920)

My bad! I didn't read the code carefully.

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

147 ↗

(On Diff #171920)

Do you really need the explicit diagnostic group? Given that there's only one diagnostic here, I would lower this into the diag (see below).

include/clang/Basic/DiagnosticSemaKinds.td

3299 ↗

(On Diff #171920)

InGroup<DiagGroup<"sizeof-pointer-div">>

lib/Sema/SemaExpr.cpp

8729 ↗

(On Diff #171920)

const Expr *

8731–8733 ↗

(On Diff #171920)

Can use const auto * here as the type is spelled out in the initialization.

Under what circumstances would you expect to get a null LHS or RHS? I suspect this should be using dyn_cast instead.

8746–8750 ↗

(On Diff #171920)

Elide braces.

test/Sema/div-sizeof-ptr.c

1 ↗

(On Diff #171920)

This is missing tests for the positive behavior -- can you add some tests involving arrays rather than pointers? Here are some other fun test cases as well:

int a[10][12];
sizeof(a) / sizeof(*a); // Should this warn?

template <typename Ty, int N>
int f(Ty (&Array)[N]) {
  return sizeof(Array) / sizeof(Ty); // Shouldn't warn
}

9 ↗

(On Diff #168539)

I think it's reasonable to not diagnose here, but I don't have strong feelings.

[xbolva00]

• Addressed review comments.

We match the GCC behaviour here, except the case:
int b1 = sizeof(int *) / sizeof(int);

But I think it is ok now.

lib/Sema/SemaExpr.cpp

8729 ↗

(On Diff #171920)

if const, i have build issues with "<< LHS"

[aaron.ballman]

LGTM with a minor formatting nit.

lib/Sema/SemaExpr.cpp

8729 ↗

(On Diff #171920)

That sounds like a bug with the diagnostic builder, but it can be addressed separately.

test/Sema/div-sizeof-ptr.cpp

1 ↗

(On Diff #172117)

Please run this file through clang-format as the formatting appears to be off.

27 ↗

(On Diff #172117)

GCC does not warn here, but I question whether we should. This evaluates to 10, but is that what the user expects it to evaluate to, as opposed to 120?

Regardless, I think it's reasonable to not warn on this right now (it would need different diagnostic text).

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

def warn_this_null_compare : Warning<
   InGroup<TautologicalUndefinedCompare>;
 def warn_address_of_reference_null_compare : Warning<
   "reference cannot be bound to dereferenced null pointer in well-defined C++ "
   "code; comparison may be assumed to always evaluate to "
   "%select{true|false}0">,
   InGroup<TautologicalUndefinedCompare>;
 def note_reference_is_return_value : Note<"%0 returns a reference">;
 
+def warn_division_sizeof_ptr : Warning<
+  "'%0' will return the size of the pointer, not the array itself">,
+  InGroup<DiagGroup<"sizeof-pointer-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">,
     InGroup<ObjCLiteralConversion>;

cfe/trunk/lib/Sema/SemaExpr.cpp

if (LHSNull == RHSNull || NonNullType->isAnyPointerType() ||
       NonNullType->canDecayToPointerType())
     return;
 
   S.Diag(Loc, diag::warn_null_in_comparison_operation)
       << LHSNull /* LHS is NULL */ << NonNullType
       << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
 }
 
+static void DiagnoseDivisionSizeofPointer(Sema &S, Expr *LHS, Expr *RHS,
+                                          SourceLocation Loc) {
+  const auto *LUE = dyn_cast<UnaryExprOrTypeTraitExpr>(LHS);
+  const auto *RUE = dyn_cast<UnaryExprOrTypeTraitExpr>(RHS);
+  if (!LUE || !RUE)
+    return;
+  if (LUE->getKind() != UETT_SizeOf || LUE->isArgumentType() ||
+      RUE->getKind() != UETT_SizeOf)
+    return;
+
+  QualType LHSTy = LUE->getArgumentExpr()->IgnoreParens()->getType();
+  QualType RHSTy;
+
+  if (RUE->isArgumentType())
+    RHSTy = RUE->getArgumentType();
+  else
+    RHSTy = RUE->getArgumentExpr()->IgnoreParens()->getType();
+
+  if (!LHSTy->isPointerType() || RHSTy->isPointerType())
+    return;
+  if (LHSTy->getPointeeType() != RHSTy)
+    return;
+
+  S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS << LHS->getSourceRange();
+}
+
 static void DiagnoseBadDivideOrRemainderValues(Sema& S, ExprResult &LHS,
                                                ExprResult &RHS,
                                                SourceLocation Loc, bool IsDiv) {
   // Check for division/remainder by zero.
   llvm::APSInt RHSValue;
   if (!RHS.get()->isValueDependent() &&
       RHS.get()->EvaluateAsInt(RHSValue, S.Context) && RHSValue == 0)
     S.DiagRuntimeBehavior(Loc, RHS.get(),
QualType Sema::CheckMultiplyDivideOperands(ExprResult &LHS, ExprResult &RHS,
 
   QualType compType = UsualArithmeticConversions(LHS, RHS, IsCompAssign);
   if (LHS.isInvalid() || RHS.isInvalid())
     return QualType();
 
 
   if (compType.isNull() || !compType->isArithmeticType())
     return InvalidOperands(Loc, LHS, RHS);
-  if (IsDiv)
+  if (IsDiv) {
     DiagnoseBadDivideOrRemainderValues(*this, LHS, RHS, Loc, IsDiv);
+    DiagnoseDivisionSizeofPointer(*this, LHS.get(), RHS.get(), Loc);
+  }
   return compType;
 }
 
 QualType Sema::CheckRemainderOperands(
   ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, bool IsCompAssign) {
   checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false);
 
   if (LHS.get()->getType()->isVectorType() ||
ExprResult Sema::ActOnObjCAvailabilityCheckExpr(
   // warn when it's used inappropriately (i.e. not if(@available)).
   if (getCurFunctionOrMethodDecl())
     getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
   else if (getCurBlock() || getCurLambda())
     getCurFunction()->HasPotentialAvailabilityViolations = true;
 
   return new (Context)
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
-}
+}
 No newline at end of file

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

+// RUN: %clang_cc1 %s -verify -Wsizeof-pointer-div -fsyntax-only
+
+template <typename Ty, int N>
+int f(Ty (&Array)[N]) {
+  return sizeof(Array) / sizeof(Ty); // Should not warn
+}
+
+void test(int *p, int **q) {
+  int a1 = sizeof(p) / sizeof(*p);   // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}
+  int a2 = sizeof p / sizeof *p;     // expected-warning {{'sizeof p' will return the size of the pointer, not the array itself}}
+  int a3 = sizeof(*q) / sizeof(**q); // expected-warning {{'sizeof (*q)' will return the size of the pointer, not the array itself}}
+  int a4 = sizeof(p) / sizeof(int);  // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}
+  int a5 = sizeof(p) / sizeof(p[0]); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}
+
+  // Should not warn
+  int b1 = sizeof(int *) / sizeof(int);
+  int b2 = sizeof(p) / sizeof(p);
+  int b3 = sizeof(*q) / sizeof(q);
+  int b4 = sizeof(p) / sizeof(char);
+
+  int arr[10];
+  int b5 = sizeof(arr) / sizeof(*arr);
+  int b6 = sizeof(arr) / sizeof(arr[0]);
+  int b7 = sizeof(arr) / sizeof(int);
+
+  int arr2[10][12];
+  int b8 = sizeof(arr2) / sizeof(*arr2);
+}