Warn on mismatching types to sizeof for memset and friends where length is of the form sizeof(Type) * factor.

[ochang]

Summary

Slightly cleaned up patch stolen from thakis@.

[thakis]

The code looks reasonable to me, but others on cfe-commits know Sema better than I.

clang generally has a high bar for adding warnings. To evaluate a warning, we usually run it on some large code base and measure true and false positives and then compare the severity of the true positives with the noise from the false positives. Do you have any data on what the warning warns on, how serious the bugs it finds are, and if there are any common patterns this warns on but shouldn't?

include/clang/Basic/DiagnosticSemaKinds.td

435 ↗

(On Diff #23373)

I wonder if this should have its own flag instead of going into the existing group – else this might be annoying for people who have the current version of this warning enabled. I'm not sure though. (Do others have opinions?)

lib/Sema/SemaChecking.cpp

4741

It feels like we should have a function for this already…I couldn't find one either though.

4758

clang uses a 2 space indent. Please run clang-format on your patch.

4916

Should this also emit a "cast to (void*) to suppress warning" fixit like other warnings here do?

If the dest is an array with a known size, and the literal is the size of the array, should we suggest sizeof(variablename) instead?

test/SemaCXX/warn-memset-bad-sizeof.cpp

4

Tests should generally be standalone and not include any headers. What do you need this for? size_t? If so, do typedef __SIZE_TYPE__ size_t; instead.

[ochang]

clang-format and remove header file dependency from test.

[ochang]

Here are some general results after some quick testing (some of which is stolen from thakis's testing on chromium). Unfortunately some of these warnings were difficult to triage and were a little confusing (which I think makes a point for this warning).

Discounting duplicates in the same file, both Firefox and Chromium give around 8-10 warnings.

Some patterns observed:

• Most seem to just be based on size assumptions between types, e.g. 2 * sizeof(uint16_t) == sizeof(uint32_t).

Firefox had about 2 instances where 2 different classes were expected to have the same size (e.g. when 1 is a wrapper around another).

• In Chromium there are some issues with structs and unions that solely contained members of a single type.

e.g.

struct M {
  int A[4];
};

M a;
memset(&a, 0, sizeof(int) * 4);

thakis@ suggested decaying the struct type to the member type in this case for the comparison.

• Firefox also had about 2 warnings related to multidimensional arrays, e.g.

typedef int Foo[10];
Foo m;
memset(&m, 0, sizeof(Foo));

Perhaps we can just compare the type 'Foo' in this case before trying to break it down further in this case.

• There are some other warnings such as differences complex_t vs float[2], w_char vs short.

[thakis]

Did the warning find any legitimate bugs?

[ochang]

Unfortunately, apart from the one mentioned in the chromium tracker, this does not seem to uncover any new legitimate bugs. Apart from Chromium and Firefox, I've tried compiling ruby and python.

lib/Sema/SemaChecking.cpp

 /// \brief If E is a sizeof expression, returns its argument type.
 static QualType getSizeOfArgType(const Expr *E, bool RequireType = false) {
   if (const UnaryExprOrTypeTraitExpr *SizeOf =
           dyn_cast<UnaryExprOrTypeTraitExpr>(E))
     if (SizeOf->getKind() == clang::UETT_SizeOf &&
         (!RequireType || SizeOf->isArgumentType()))
       return SizeOf->getTypeOfArgument();

 static bool isSizeOfArgFactorTypeCompatible(ASTContext &Context,
                                             QualType &SizeOfArgFactorTy,
                                             QualType &EltType) {
-    QualType LHSType =
-        Context.getCanonicalType(SizeOfArgFactorTy).getUnqualifiedType();
-    QualType RHSType =
-        Context.getCanonicalType(EltType).getUnqualifiedType();
+  QualType RHSType = Context.getCanonicalType(EltType).getUnqualifiedType();
 
-    if (LHSType->hasSignedIntegerRepresentation() &&
-        LHSType != Context.getWCharType())
-      LHSType = Context.getCorrespondingUnsignedType(LHSType);
 
-    if (RHSType->hasSignedIntegerRepresentation() &&
-        RHSType != Context.getWCharType())
-      RHSType = Context.getCorrespondingUnsignedType(RHSType);
 
-    return typesAreCompatibleIgnoreQualifiers(Context, LHSType, RHSType);
 }
 
 /// \brief Check for dangerous or invalid arguments to memset().

   const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
 
   QualType SizeOfArgFactorTy;
-  const Expr* OtherSizeOfFactor;
+  const Expr *OtherSizeOfFactor;
   bool SizeOfIsFactor =
       getSizeOfArgTypeProduct(LenExpr, SizeOfArgFactorTy, OtherSizeOfFactor);
 

test/SemaCXX/warn-memset-bad-sizeof.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-sizeof-array-argument %s
 //
 
-#include <stddef.h>
+typedef __SIZE_TYPE__ size_t;
 
 extern "C" void *memset(void *, int, unsigned);
 extern "C" void *memmove(void *s1, const void *s2, unsigned n);