On Sat, Jul 21, 2018 at 4:40 PM, Arthur O'Dwyer via cfe-commits <cfe-commits@lists.llvm.org> wrote: > In this case Clang is complaining about > > memset(complicated-expr, 0, 0); > > which means that transposing the last two arguments would not "fix" > anything. Clang ought to not complain in this case.
+1 ~Aaron > It doesn't have anything to do with coming from a macro; it's just that both > arguments are zero. > (I would also expect that `memset(complicated-expr, (unsigned char)fill, > (size_t)0)` shouldn't warn, either. But we had some disagreement that casts > to `(unsigned char)` resp. `(size_t)` should be the right way to shut up the > warning in the first place...) > Basically Clang should only be suggesting a swap (and thus complaining at > all) in the case that swapping the arguments would actually improve the > situation. > > –Arthur > > On Sat, Jul 21, 2018 at 1:33 PM, Nico Weber via cfe-commits > <cfe-commits@lists.llvm.org> wrote: >> >> This has a false positive on ffmpeg: >> >> ../../third_party/ffmpeg/libavcodec/options.c:273:64: error: 'size' >> argument to memset is '0'; did you mean to transpose the last two arguments? >> [-Werror,-Wmemset-transposed-args] >> alloc_and_copy_or_fail(intra_matrix, 64 * sizeof(int16_t), 0); >> >> With: >> >> #define alloc_and_copy_or_fail(obj, size, pad) \ >> if (src->obj && size > 0) { \ >> dest->obj = av_malloc(size + pad); \ >> if (!dest->obj) \ >> goto fail; \ >> memcpy(dest->obj, src->obj, size); \ >> if (pad) \ >> memset(((uint8_t *) dest->obj) + size, 0, pad); \ >> } >> >> >> (https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/options.c?q=alloc_and_copy_or_fail&sq=package:chromium&g=0&l=261 >> ; https://bugs.chromium.org/p/chromium/issues/detail?id=866202) >> >> Maybe the warning shouldn't fire if the memset is from a macro? >> >> On Thu, Jul 19, 2018 at 12:51 PM Erik Pilkington via cfe-commits >> <cfe-commits@lists.llvm.org> wrote: >>> >>> Author: epilk >>> Date: Thu Jul 19 09:46:15 2018 >>> New Revision: 337470 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=337470&view=rev >>> Log: >>> [Sema] Add a new warning, -Wmemset-transposed-args >>> >>> This diagnoses calls to memset that have the second and third arguments >>> transposed, for example: >>> >>> memset(buf, sizeof(buf), 0); >>> >>> This is done by checking if the third argument is a literal 0, or if the >>> second >>> is a sizeof expression (and the third isn't). The first check is also >>> done for >>> calls to bzero. >>> >>> Differential revision: https://reviews.llvm.org/D49112 >>> >>> Added: >>> cfe/trunk/test/Sema/transpose-memset.c >>> Modified: >>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td >>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >>> cfe/trunk/lib/Sema/SemaChecking.cpp >>> >>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=337470&r1=337469&r2=337470&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) >>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Jul 19 09:46:15 >>> 2018 >>> @@ -443,6 +443,13 @@ def : DiagGroup<"synth">; >>> def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">; >>> def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">; >>> def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">; >>> +def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">; >>> +def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">; >>> +def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">; >>> +def SuspiciousBzero : DiagGroup<"suspicious-bzero">; >>> +def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess", >>> + [SizeofPointerMemaccess, DynamicClassMemaccess, >>> + NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>; >>> def StaticInInline : DiagGroup<"static-in-inline">; >>> def StaticLocalInInline : DiagGroup<"static-local-in-inline">; >>> def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">; >>> >>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=337470&r1=337469&r2=337470&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 19 >>> 09:46:15 2018 >>> @@ -619,14 +619,14 @@ def warn_cstruct_memaccess : Warning< >>> "%select{destination for|source of|first operand of|second operand >>> of}0 this " >>> "%1 call is a pointer to record %2 that is not trivial to " >>> "%select{primitive-default-initialize|primitive-copy}3">, >>> - InGroup<DiagGroup<"nontrivial-memaccess">>; >>> + InGroup<NonTrivialMemaccess>; >>> def note_nontrivial_field : Note< >>> "field is non-trivial to %select{copy|default-initialize}0">; >>> def warn_dyn_class_memaccess : Warning< >>> "%select{destination for|source of|first operand of|second operand >>> of}0 this " >>> "%1 call is a pointer to %select{|class containing a }2dynamic class >>> %3; " >>> "vtable pointer will be %select{overwritten|copied|moved|compared}4">, >>> - InGroup<DiagGroup<"dynamic-class-memaccess">>; >>> + InGroup<DynamicClassMemaccess>; >>> def note_bad_memaccess_silence : Note< >>> "explicitly cast the pointer to silence this warning">; >>> def warn_sizeof_pointer_expr_memaccess : Warning< >>> @@ -655,7 +655,19 @@ def note_memsize_comparison_paren : Note >>> "did you mean to compare the result of %0 instead?">; >>> def note_memsize_comparison_cast_silence : Note< >>> "explicitly cast the argument to size_t to silence this warning">; >>> - >>> +def warn_suspicious_sizeof_memset : Warning< >>> + "%select{'size' argument to memset is '0'|" >>> + "setting buffer to a 'sizeof' expression}0" >>> + "; did you mean to transpose the last two arguments?">, >>> + InGroup<MemsetTransposedArgs>; >>> +def note_suspicious_sizeof_memset_silence : Note< >>> + "%select{parenthesize the third argument|" >>> + "cast the second argument to 'int'}0 to silence">; >>> +def warn_suspicious_bzero_size : Warning<"'size' argument to bzero is >>> '0'">, >>> + InGroup<SuspiciousBzero>; >>> +def note_suspicious_bzero_size_silence : Note< >>> + "parenthesize the second argument to silence">; >>> + >>> def warn_strncat_large_size : Warning< >>> "the value of the size argument in 'strncat' is too large, might lead >>> to a " >>> "buffer overflow">, InGroup<StrncatSize>; >>> >>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=337470&r1=337469&r2=337470&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Jul 19 09:46:15 2018 >>> @@ -8629,24 +8629,26 @@ static const CXXRecordDecl *getContained >>> return nullptr; >>> } >>> >>> +static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) { >>> + if (const auto *Unary = dyn_cast<UnaryExprOrTypeTraitExpr>(E)) >>> + if (Unary->getKind() == UETT_SizeOf) >>> + return Unary; >>> + return nullptr; >>> +} >>> + >>> /// If E is a sizeof expression, returns its argument expression, >>> /// otherwise returns NULL. >>> static const Expr *getSizeOfExprArg(const Expr *E) { >>> - if (const UnaryExprOrTypeTraitExpr *SizeOf = >>> - dyn_cast<UnaryExprOrTypeTraitExpr>(E)) >>> - if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType()) >>> + if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E)) >>> + if (!SizeOf->isArgumentType()) >>> return SizeOf->getArgumentExpr()->IgnoreParenImpCasts(); >>> - >>> return nullptr; >>> } >>> >>> /// If E is a sizeof expression, returns its argument type. >>> static QualType getSizeOfArgType(const Expr *E) { >>> - if (const UnaryExprOrTypeTraitExpr *SizeOf = >>> - dyn_cast<UnaryExprOrTypeTraitExpr>(E)) >>> - if (SizeOf->getKind() == UETT_SizeOf) >>> - return SizeOf->getTypeOfArgument(); >>> - >>> + if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E)) >>> + return SizeOf->getTypeOfArgument(); >>> return QualType(); >>> } >>> >>> @@ -8742,6 +8744,86 @@ struct SearchNonTrivialToCopyField >>> >>> } >>> >>> +/// Detect if \c SizeofExpr is likely to calculate the sizeof an object. >>> +static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) { >>> + SizeofExpr = SizeofExpr->IgnoreParenImpCasts(); >>> + >>> + if (const auto *BO = dyn_cast<BinaryOperator>(SizeofExpr)) { >>> + if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add) >>> + return false; >>> + >>> + return doesExprLikelyComputeSize(BO->getLHS()) || >>> + doesExprLikelyComputeSize(BO->getRHS()); >>> + } >>> + >>> + return getAsSizeOfExpr(SizeofExpr) != nullptr; >>> +} >>> + >>> +/// Check if the ArgLoc originated from a macro passed to the call at >>> CallLoc. >>> +/// >>> +/// \code >>> +/// #define MACRO 0 >>> +/// foo(MACRO); >>> +/// foo(0); >>> +/// \endcode >>> +/// >>> +/// This should return true for the first call to foo, but not for the >>> second >>> +/// (regardless of whether foo is a macro or function). >>> +static bool isArgumentExpandedFromMacro(SourceManager &SM, >>> + SourceLocation CallLoc, >>> + SourceLocation ArgLoc) { >>> + if (!CallLoc.isMacroID()) >>> + return SM.getFileID(CallLoc) != SM.getFileID(ArgLoc); >>> + >>> + return SM.getFileID(SM.getImmediateMacroCallerLoc(CallLoc)) != >>> + SM.getFileID(SM.getImmediateMacroCallerLoc(ArgLoc)); >>> +} >>> + >>> +/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have >>> the >>> +/// last two arguments transposed. >>> +static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr >>> *Call) { >>> + if (BId != Builtin::BImemset && BId != Builtin::BIbzero) >>> + return; >>> + >>> + const Expr *SizeArg = >>> + Call->getArg(BId == Builtin::BImemset ? 2 : 1)->IgnoreImpCasts(); >>> + >>> + // If we're memsetting or bzeroing 0 bytes, then this is likely an >>> error. >>> + SourceLocation CallLoc = Call->getRParenLoc(); >>> + SourceManager &SM = S.getSourceManager(); >>> + if (isa<IntegerLiteral>(SizeArg) && >>> + cast<IntegerLiteral>(SizeArg)->getValue() == 0 && >>> + !isArgumentExpandedFromMacro(SM, CallLoc, SizeArg->getExprLoc())) >>> { >>> + >>> + SourceLocation DiagLoc = SizeArg->getExprLoc(); >>> + >>> + // Some platforms #define bzero to __builtin_memset. See if this is >>> the >>> + // case, and if so, emit a better diagnostic. >>> + if (BId == Builtin::BIbzero || >>> + (CallLoc.isMacroID() && Lexer::getImmediateMacroName( >>> + CallLoc, SM, S.getLangOpts()) == >>> "bzero")) { >>> + S.Diag(DiagLoc, diag::warn_suspicious_bzero_size); >>> + S.Diag(DiagLoc, diag::note_suspicious_bzero_size_silence); >>> + } else { >>> + S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 0; >>> + S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 0; >>> + } >>> + return; >>> + } >>> + >>> + // If the second argument to a memset is a sizeof expression and the >>> third >>> + // isn't, this is also likely an error. This should catch >>> + // 'memset(buf, sizeof(buf), 0xff)'. >>> + if (BId == Builtin::BImemset && >>> + doesExprLikelyComputeSize(Call->getArg(1)) && >>> + !doesExprLikelyComputeSize(Call->getArg(2))) { >>> + SourceLocation DiagLoc = Call->getArg(1)->getExprLoc(); >>> + S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 1; >>> + S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 1; >>> + return; >>> + } >>> +} >>> + >>> /// Check for dangerous or invalid arguments to memset(). >>> /// >>> /// This issues warnings on known problematic, dangerous or unspecified >>> @@ -8771,6 +8853,9 @@ void Sema::CheckMemaccessArguments(const >>> Call->getLocStart(), >>> Call->getRParenLoc())) >>> return; >>> >>> + // Catch cases like 'memset(buf, sizeof(buf), 0)'. >>> + CheckMemaccessSize(*this, BId, Call); >>> + >>> // We have special checking when the length is a sizeof expression. >>> QualType SizeOfArgTy = getSizeOfArgType(LenExpr); >>> const Expr *SizeOfArg = getSizeOfExprArg(LenExpr); >>> >>> Added: cfe/trunk/test/Sema/transpose-memset.c >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/transpose-memset.c?rev=337470&view=auto >>> >>> ============================================================================== >>> --- cfe/trunk/test/Sema/transpose-memset.c (added) >>> +++ cfe/trunk/test/Sema/transpose-memset.c Thu Jul 19 09:46:15 2018 >>> @@ -0,0 +1,60 @@ >>> +// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s >>> +// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s >>> + >>> +#define memset(...) __builtin_memset(__VA_ARGS__) >>> +#define bzero(x,y) __builtin_memset(x, 0, y) >>> +#define real_bzero(x,y) __builtin_bzero(x,y) >>> + >>> +int array[10]; >>> +int *ptr; >>> + >>> +int main() { >>> + memset(array, sizeof(array), 0); // expected-warning{{'size' argument >>> to memset is '0'; did you mean to transpose the last two arguments?}} >>> expected-note{{parenthesize the third argument to silence}} >>> + memset(array, sizeof(array), 0xff); // expected-warning{{setting >>> buffer to a 'sizeof' expression; did you mean to transpose the last two >>> arguments?}} expected-note{{cast the second argument to 'int' to silence}} >>> + memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to >>> memset is '0'; did you mean to transpose the last two arguments?}} >>> expected-note{{parenthesize the third argument to silence}} >>> + memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer >>> to a 'sizeof' expression; did you mean to transpose the last two >>> arguments?}} expected-note{{cast the second argument to 'int' to silence}} >>> + memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting >>> buffer to a 'sizeof' expression; did you mean to transpose the last two >>> arguments?}} expected-note{{cast the second argument to 'int' to silence}} >>> + memset(ptr, 10 * sizeof(int *) + 10, 0xff); // >>> expected-warning{{setting buffer to a 'sizeof' expression; did you mean to >>> transpose the last two arguments?}} expected-note{{cast the second argument >>> to 'int' to silence}} >>> + memset(ptr, sizeof(char) * sizeof(int *), 0xff); // >>> expected-warning{{setting buffer to a 'sizeof' expression; did you mean to >>> transpose the last two arguments?}} expected-note{{cast the second argument >>> to 'int' to silence}} >>> + memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess. >>> + memset(array, 0, sizeof(array)); >>> + memset(ptr, 0, sizeof(int *) * 10); >>> + memset(array, (int)sizeof(array), (0)); // no warning >>> + memset(array, (int)sizeof(array), 32); // no warning >>> + memset(array, 32, (0)); // no warning >>> + >>> + bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} >>> expected-note{{parenthesize the second argument to silence}} >>> + real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is >>> '0'}} expected-note{{parenthesize the second argument to silence}} >>> +} >>> + >>> +void macros() { >>> +#define ZERO 0 >>> + int array[10]; >>> + memset(array, 0xff, ZERO); // no warning >>> + // Still emit a diagnostic for memsetting a sizeof expression: >>> + memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} >>> expected-note{{cast}} >>> + bzero(array, ZERO); // no warning >>> + real_bzero(array, ZERO); // no warning >>> +#define NESTED_DONT_DIAG \ >>> + memset(array, 0xff, ZERO); \ >>> + real_bzero(array, ZERO); >>> + >>> + NESTED_DONT_DIAG; >>> + >>> +#define NESTED_DO_DIAG \ >>> + memset(array, 0xff, 0); \ >>> + real_bzero(array, 0) >>> + >>> + NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} >>> expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}} >>> + >>> +#define FN_MACRO(p) \ >>> + memset(array, 0xff, p) >>> + >>> + FN_MACRO(ZERO); >>> + FN_MACRO(0); // FIXME: should we diagnose this? >>> + >>> + __builtin_memset(array, 0, ZERO); // no warning >>> + __builtin_bzero(array, ZERO); >>> + __builtin_memset(array, 0, 0); // expected-warning{{'size' argument to >>> memset}} // expected-note{{parenthesize}} >>> + __builtin_bzero(array, 0); // expected-warning{{'size' argument to >>> bzero}} // expected-note{{parenthesize}} >>> +} >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits