[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

10

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

14

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

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

145

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

3296

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

lib/Sema/SemaExpr.cpp

8680

const Expr *

8682–8684

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.

8697–8701

Elide braces.

test/Sema/div-sizeof-ptr.c

2

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
}

10

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

8680

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

[aaron.ballman]

LGTM with a minor formatting nit.

lib/Sema/SemaExpr.cpp

8680

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).

include/clang/Basic/DiagnosticGroups.td

 def DynamicExceptionSpec
     : DiagGroup<"dynamic-exception-spec", [DeprecatedDynamicExceptionSpec]>;
 
 def LibLTO : DiagGroup<"liblto">;
 def : DiagGroup<"disabled-optimization">;
 def : DiagGroup<"discard-qual">;
 def DivZero : DiagGroup<"division-by-zero">;
 def : DiagGroup<"div-by-zero", [DivZero]>;
+def DivSizeofPtr : DiagGroup<"sizeof-pointer-div">;
 
 def DocumentationHTML : DiagGroup<"documentation-html">;
 def DocumentationUnknownCommand : DiagGroup<"documentation-unknown-command">;
 def DocumentationPedantic : DiagGroup<"documentation-pedantic",
                                       [DocumentationUnknownCommand]>;
 def DocumentationDeprecatedSync : DiagGroup<"documentation-deprecated-sync">;
 def Documentation : DiagGroup<"documentation",
                               [DocumentationHTML,
def Most : DiagGroup<"most", [
     Move,
     MultiChar,
     Reorder,
     ReturnType,
     SelfAssignment,
     SelfMove,
     SizeofArrayArgument,
     SizeofArrayDecay,
+    DivSizeofPtr,
     StringPlusInt,
     Trigraphs,
     Uninitialized,
     UnknownPragmas,
     Unused,
     VolatileRegisterVar,
     ObjCMissingSuperCalls,
     ObjCDesignatedInit,

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<
+  "division produces the incorrect number of array elements">,
+  InGroup<DivSizeofPtr>;
+
 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>;

lib/Sema/SemaExpr.cpp

CastKind Sema::PrepareScalarCast(ExprResult &Src, QualType DestTy) {
   case Type::STK_BlockPointer:
   case Type::STK_ObjCObjectPointer:
     switch (DestTy->getScalarTypeKind()) {
     case Type::STK_CPointer: {
       LangAS SrcAS = SrcTy->getPointeeType().getAddressSpace();
       LangAS DestAS = DestTy->getPointeeType().getAddressSpace();
       if (SrcAS != DestAS)
         return CK_AddressSpaceConversion;
+      if (Context.hasCvrSimilarType(SrcTy, DestTy))
+        return CK_NoOp;
       return CK_BitCast;
     }
     case Type::STK_BlockPointer:
       return (SrcKind == Type::STK_BlockPointer
                 ? CK_BitCast : CK_AnyPointerToBlockPointerCast);
     case Type::STK_ObjCObjectPointer:
       if (SrcKind == Type::STK_ObjCObjectPointer)
         return CK_BitCast;
Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS,
   }
 
   // Conversions to normal pointers.
   if (const PointerType *LHSPointer = dyn_cast<PointerType>(LHSType)) {
     // U* -> T*
     if (isa<PointerType>(RHSType)) {
       LangAS AddrSpaceL = LHSPointer->getPointeeType().getAddressSpace();
       LangAS AddrSpaceR = RHSType->getPointeeType().getAddressSpace();
-      Kind = AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+      if (AddrSpaceL != AddrSpaceR)
+        Kind = CK_AddressSpaceConversion;
+      else if (Context.hasCvrSimilarType(RHSType, LHSType))
+        Kind = CK_NoOp;
+      else
+        Kind = CK_BitCast;
       return checkPointerTypesForAssignment(*this, LHSType, RHSType);
     }
 
     // int -> T*
     if (RHSType->isIntegerType()) {
       Kind = CK_IntegralToPointer; // FIXME: null?
       return IntToPointer;
     }
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) {
+  UnaryExprOrTypeTraitExpr *LUE =
+      dyn_cast_or_null<UnaryExprOrTypeTraitExpr>(LHS);
+  UnaryExprOrTypeTraitExpr *RUE =
+      dyn_cast_or_null<UnaryExprOrTypeTraitExpr>(RHS);
+
+  if (!LUE || !RUE)
+    return;
+  if (LUE->getKind() != UETT_SizeOf || LUE->isArgumentType() ||
+      RUE->getKind() != UETT_SizeOf)
+    return;
+
+  QualType LHSTy;
+  QualType RHSTy;
+  LHS = LUE->getArgumentExpr()->IgnoreParens();
+  LHSTy = LHS->getType();
+
+  if (RUE->isArgumentType()) {
+    RHSTy = RUE->getArgumentType();
+  } else {
+    RHS = RUE->getArgumentExpr()->IgnoreParens();
+    RHSTy = RHS->getType();
+  }
+
+  if (!LHSTy->isPointerType() || RHSTy->isPointerType())
+    return;
+  if (LHSTy->getPointeeType() != RHSTy)
+    return;
+
+  S.Diag(Loc, diag::warn_division_sizeof_ptr) << 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::ActOnBlockStmtExpr(SourceLocation CaretLoc,
   // Leave the expression-evaluation context.
   if (hasAnyUnrecoverableErrorsInThisFunction())
     DiscardCleanupsInEvaluationContext();
   assert(!Cleanup.exprNeedsCleanups() &&
          "cleanups within block not correctly bound!");
   PopExpressionEvaluationContext();
 
   BlockScopeInfo *BSI = cast<BlockScopeInfo>(FunctionScopes.back());
+  BlockDecl *BD = BSI->TheDecl;
 
   if (BSI->HasImplicitReturnType)
     deduceClosureReturnType(*BSI);
 
   PopDeclContext();
 
   QualType RetTy = Context.VoidTy;
   if (!BSI->ReturnType.isNull())
     RetTy = BSI->ReturnType;
 
-  bool NoReturn = BSI->TheDecl->hasAttr<NoReturnAttr>();
+  bool NoReturn = BD->hasAttr<NoReturnAttr>();
   QualType BlockTy;
 
   // Set the captured variables on the block.
   // FIXME: Share capture structure between BlockDecl and CapturingScopeInfo!
   SmallVector<BlockDecl::Capture, 4> Captures;
   for (Capture &Cap : BSI->Captures) {
     if (Cap.isThisCapture())
       continue;
     BlockDecl::Capture NewCap(Cap.getVariable(), Cap.isBlockCapture(),
                               Cap.isNested(), Cap.getInitExpr());
     Captures.push_back(NewCap);
   }
-  BSI->TheDecl->setCaptures(Context, Captures, BSI->CXXThisCaptureIndex != 0);
+  BD->setCaptures(Context, Captures, BSI->CXXThisCaptureIndex != 0);
 
   // If the user wrote a function type in some form, try to use that.
   if (!BSI->FunctionType.isNull()) {
     const FunctionType *FTy = BSI->FunctionType->getAs<FunctionType>();
 
     FunctionType::ExtInfo Ext = FTy->getExtInfo();
     if (NoReturn && !Ext.getNoReturn()) Ext = Ext.withNoReturn(true);
 
ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
 
   // If we don't have a function type, just build one from nothing.
   } else {
     FunctionProtoType::ExtProtoInfo EPI;
     EPI.ExtInfo = FunctionType::ExtInfo().withNoReturn(NoReturn);
     BlockTy = Context.getFunctionType(RetTy, None, EPI);
   }
 
-  DiagnoseUnusedParameters(BSI->TheDecl->parameters());
+  DiagnoseUnusedParameters(BD->parameters());
   BlockTy = Context.getBlockPointerType(BlockTy);
 
   // If needed, diagnose invalid gotos and switches in the block.
   if (getCurFunction()->NeedsScopeChecking() &&
       !PP.isCodeCompletionEnabled())
     DiagnoseInvalidJumps(cast<CompoundStmt>(Body));
 
-  BSI->TheDecl->setBody(cast<CompoundStmt>(Body));
+  BD->setBody(cast<CompoundStmt>(Body));
 
   if (Body && getCurFunction()->HasPotentialAvailabilityViolations)
-    DiagnoseUnguardedAvailabilityViolations(BSI->TheDecl);
+    DiagnoseUnguardedAvailabilityViolations(BD);
 
   // Try to apply the named return value optimization. We have to check again
   // if we can do this, though, because blocks keep return statements around
   // to deduce an implicit return type.
   if (getLangOpts().CPlusPlus && RetTy->isRecordType() &&
-      !BSI->TheDecl->isDependentContext())
+      !BD->isDependentContext())
     computeNRVO(Body, BSI);
 
-  BlockExpr *Result = new (Context) BlockExpr(BSI->TheDecl, BlockTy);
+  BlockExpr *Result = new (Context) BlockExpr(BD, BlockTy);
   AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
   PopFunctionScopeInfo(&WP, Result->getBlockDecl(), Result);
 
   // If the block isn't obviously global, i.e. it captures anything at
   // all, then we need to do a few things in the surrounding context:
   if (Result->getBlockDecl()->hasCaptures()) {
     // First, this expression has a new cleanup object.
     ExprCleanupObjects.push_back(Result->getBlockDecl());
     Cleanup.setExprNeedsCleanups(true);
 
     // It also gets a branch-protected scope if any of the captured
     // variables needs destruction.
     for (const auto &CI : Result->getBlockDecl()->captures()) {
       const VarDecl *var = CI.getVariable();
       if (var->getType().isDestructedType() != QualType::DK_none) {
         setFunctionHasBranchProtectedScope();
         break;
       }
     }
   }
 
   if (getCurFunction())
-    getCurFunction()->addBlock(BSI->TheDecl);
+    getCurFunction()->addBlock(BD);
 
   return Result;
 }
 
 ExprResult Sema::ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,
                             SourceLocation RPLoc) {
   TypeSourceInfo *TInfo;
   GetTypeFromParser(Ty, &TInfo);
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

test/Sema/div-sizeof-ptr.c

+// RUN: %clang_cc1 %s -verify -Wsizeof-pointer-div -fsyntax-only
+
+void test(int *p, int **q) {
+    int a = sizeof(p) / sizeof(*p);      // expected-warning {{division produces the incorrect number of array elements}}
+    int b = sizeof p / sizeof *p;        // expected-warning {{division produces the incorrect number of array elements}}
+    int c = sizeof(*q) / sizeof(**q);    // expected-warning {{division produces the incorrect number of array elements}}
+    int d = sizeof(p) / sizeof(int);     // expected-warning {{division produces the incorrect number of array elements}}
+    
+    int e = sizeof(int *) / sizeof(int);
+    int f = sizeof(p) / sizeof(p);
+    int g = sizeof(*q) / sizeof(q);
+}