https://github.com/tynasello created https://github.com/llvm/llvm-project/pull/154432
The diagnose_as_builtin attribute can be used to request that Clang diagnose the annotated function as though it was a (user-specified) builtin. This doesn't apply for -Wmemset-transposed-args as seen [here](https://godbolt.org/z/cc36TTGWW). This fix adds the diagnosis for this case. >From 71b7faef3d302f62eafa2976c9587cc27419c36d Mon Sep 17 00:00:00 2001 From: Ty Nasello <tynase...@gmail.com> Date: Mon, 18 Aug 2025 10:12:00 -0700 Subject: [PATCH] [Clang] Fix to memset diagnostic Clang has a diagnose_as_builtin function, which requests Clang to diagnose the annotated function as though it was a builtin that the user can select. This doesn't apply for -Wmemset-transposed-args. This fix makes diagnose_as_builtin diagnose this case. --- clang/include/clang/AST/Expr.h | 26 ++++++ clang/include/clang/Sema/Sema.h | 9 +- clang/lib/AST/Expr.cpp | 22 +++++ clang/lib/Sema/SemaChecking.cpp | 136 +++++++++++++---------------- clang/test/Sema/transpose-memset.c | 9 ++ 5 files changed, 122 insertions(+), 80 deletions(-) diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index 708c6e2925fd0..4ee73f678fe4b 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -3285,6 +3285,32 @@ class CallExpr : public Expr { } }; +class BuiltInLikeCall { + /// If the CallExpr is to a function that has a DiagnoseAsBuiltinAttr + /// attribute, then FDecl will be the function declaration in that attribute. + /// Otherwise, FDecl will be the function declaration for the CallExpr. + const FunctionDecl *FDecl; + const CallExpr *TheCall; + const DiagnoseAsBuiltinAttr *DABAttr; + +public: + BuiltInLikeCall(const FunctionDecl *FD, const CallExpr *TheCall) + : TheCall(TheCall) { + DABAttr = FD->getAttr<DiagnoseAsBuiltinAttr>(); + if (DABAttr) { + FDecl = DABAttr->getFunction(); + assert(FDecl && "Missing FunctionDecl in DiagnoseAsBuiltin attribute!"); + } else { + FDecl = FD; + } + } + const FunctionDecl *getFDecl() const { return FDecl; } + const CallExpr *getCall() const { return TheCall; } + const DiagnoseAsBuiltinAttr *getDABAttr() const { return DABAttr; } + std::optional<unsigned> TranslateIndex(unsigned Index) const; + const Expr *getNonVariadicArg(unsigned Arg) const; +}; + /// MemberExpr - [C99 6.5.2.3] Structure and Union Members. X->F and X.F. /// class MemberExpr final diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 423dcf9e2b708..5a7599db752a3 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -3039,21 +3039,22 @@ class Sema final : public SemaBase { /// function calls. /// /// \param Call The call expression to diagnose. - void CheckMemaccessArguments(const CallExpr *Call, unsigned BId, + void CheckMemaccessArguments(const BuiltInLikeCall &BILC, unsigned BId, IdentifierInfo *FnName); // Warn if the user has made the 'size' argument to strlcpy or strlcat // be the size of the source, instead of the destination. - void CheckStrlcpycatArguments(const CallExpr *Call, IdentifierInfo *FnName); + void CheckStrlcpycatArguments(const BuiltInLikeCall &BILC, + IdentifierInfo *FnName); // Warn on anti-patterns as the 'size' argument to strncat. // The correct size argument should look like following: // strncat(dst, src, sizeof(dst) - strlen(dest) - 1); - void CheckStrncatArguments(const CallExpr *Call, + void CheckStrncatArguments(const BuiltInLikeCall &BILC, const IdentifierInfo *FnName); /// Alerts the user that they are attempting to free a non-malloc'd object. - void CheckFreeArguments(const CallExpr *E); + void CheckFreeArguments(const BuiltInLikeCall &BILC); void CheckReturnValExpr(Expr *RetValExp, QualType lhsType, SourceLocation ReturnLoc, bool isObjCMethod = false, diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index d85655b1596f4..523588c1188ed 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -1706,6 +1706,28 @@ UnaryExprOrTypeTraitExpr::UnaryExprOrTypeTraitExpr( setDependence(computeDependence(this)); } +std::optional<unsigned> BuiltInLikeCall::TranslateIndex(unsigned Index) const { + // If we refer to a diagnose_as_builtin attribute, we need to change the + // argument index to refer to the arguments of the called function. Unless + // the index is out of bounds, which presumably means it's a variadic + // function. + if (!getDABAttr()) + return Index; + unsigned DABIndices = getDABAttr()->argIndices_size(); + unsigned NewIndex = Index < DABIndices + ? getDABAttr()->argIndices_begin()[Index] + : Index - DABIndices + getFDecl()->getNumParams(); + if (NewIndex >= TheCall->getNumArgs()) + return std::nullopt; + return NewIndex; +} + +const Expr *BuiltInLikeCall::getNonVariadicArg(unsigned Index) const { + auto NewIndex = TranslateIndex(Index); + assert(NewIndex && "Translated arg access is out of range!"); + return TheCall->getArg(NewIndex.value()); +} + MemberExpr::MemberExpr(Expr *Base, bool IsArrow, SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc, SourceLocation TemplateKWLoc, ValueDecl *MemberDecl, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index c74b67106ad74..cd29335fdf754 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1142,17 +1142,9 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, isConstantEvaluatedContext()) return; - bool UseDABAttr = false; - const FunctionDecl *UseDecl = FD; + const BuiltInLikeCall BILC(FD, TheCall); - const auto *DABAttr = FD->getAttr<DiagnoseAsBuiltinAttr>(); - if (DABAttr) { - UseDecl = DABAttr->getFunction(); - assert(UseDecl && "Missing FunctionDecl in DiagnoseAsBuiltin attribute!"); - UseDABAttr = true; - } - - unsigned BuiltinID = UseDecl->getBuiltinID(/*ConsiderWrappers=*/true); + unsigned BuiltinID = BILC.getFDecl()->getBuiltinID(/*ConsiderWrappers=*/true); if (!BuiltinID) return; @@ -1160,25 +1152,9 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, const TargetInfo &TI = getASTContext().getTargetInfo(); unsigned SizeTypeWidth = TI.getTypeWidth(TI.getSizeType()); - auto TranslateIndex = [&](unsigned Index) -> std::optional<unsigned> { - // If we refer to a diagnose_as_builtin attribute, we need to change the - // argument index to refer to the arguments of the called function. Unless - // the index is out of bounds, which presumably means it's a variadic - // function. - if (!UseDABAttr) - return Index; - unsigned DABIndices = DABAttr->argIndices_size(); - unsigned NewIndex = Index < DABIndices - ? DABAttr->argIndices_begin()[Index] - : Index - DABIndices + FD->getNumParams(); - if (NewIndex >= TheCall->getNumArgs()) - return std::nullopt; - return NewIndex; - }; - auto ComputeExplicitObjectSizeArgument = [&](unsigned Index) -> std::optional<llvm::APSInt> { - std::optional<unsigned> IndexOptional = TranslateIndex(Index); + std::optional<unsigned> IndexOptional = BILC.TranslateIndex(Index); if (!IndexOptional) return std::nullopt; unsigned NewIndex = *IndexOptional; @@ -1204,7 +1180,7 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, BOSType = POS->getType(); } - std::optional<unsigned> IndexOptional = TranslateIndex(Index); + std::optional<unsigned> IndexOptional = BILC.TranslateIndex(Index); if (!IndexOptional) return std::nullopt; unsigned NewIndex = *IndexOptional; @@ -1223,7 +1199,7 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, auto ComputeStrLenArgument = [&](unsigned Index) -> std::optional<llvm::APSInt> { - std::optional<unsigned> IndexOptional = TranslateIndex(Index); + std::optional<unsigned> IndexOptional = BILC.TranslateIndex(Index); if (!IndexOptional) return std::nullopt; unsigned NewIndex = *IndexOptional; @@ -3799,24 +3775,25 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall, if (getLangOpts().ObjC) ObjC().DiagnoseCStringFormatDirectiveInCFAPI(FDecl, Args, NumArgs); - unsigned CMId = FDecl->getMemoryFunctionKind(); - // Handle memory setting and copying functions. + BuiltInLikeCall BILC(FDecl, TheCall); + unsigned CMId = BILC.getFDecl()->getMemoryFunctionKind(); + switch (CMId) { case 0: return false; case Builtin::BIstrlcpy: // fallthrough case Builtin::BIstrlcat: - CheckStrlcpycatArguments(TheCall, FnInfo); + CheckStrlcpycatArguments(BILC, FnInfo); break; case Builtin::BIstrncat: - CheckStrncatArguments(TheCall, FnInfo); + CheckStrncatArguments(BILC, FnInfo); break; case Builtin::BIfree: - CheckFreeArguments(TheCall); + CheckFreeArguments(BILC); break; default: - CheckMemaccessArguments(TheCall, CMId, FnInfo); + CheckMemaccessArguments(BILC, CMId, FnInfo); } return false; @@ -9727,12 +9704,13 @@ static bool isArgumentExpandedFromMacro(SourceManager &SM, /// 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) { +static void CheckMemaccessSize(Sema &S, unsigned BId, + const BuiltInLikeCall &BILC) { if (BId != Builtin::BImemset && BId != Builtin::BIbzero) return; - const Expr *SizeArg = - Call->getArg(BId == Builtin::BImemset ? 2 : 1)->IgnoreImpCasts(); + const Expr *SizeArg = BILC.getNonVariadicArg(BId == Builtin::BImemset ? 2 : 1) + ->IgnoreImpCasts(); auto isLiteralZero = [](const Expr *E) { return (isa<IntegerLiteral>(E) && @@ -9742,7 +9720,7 @@ static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr *Call) { }; // If we're memsetting or bzeroing 0 bytes, then this is likely an error. - SourceLocation CallLoc = Call->getRParenLoc(); + SourceLocation CallLoc = BILC.getCall()->getRParenLoc(); SourceManager &SM = S.getSourceManager(); if (isLiteralZero(SizeArg) && !isArgumentExpandedFromMacro(SM, CallLoc, SizeArg->getExprLoc())) { @@ -9756,7 +9734,7 @@ static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr *Call) { CallLoc, SM, S.getLangOpts()) == "bzero")) { S.Diag(DiagLoc, diag::warn_suspicious_bzero_size); S.Diag(DiagLoc, diag::note_suspicious_bzero_size_silence); - } else if (!isLiteralZero(Call->getArg(1)->IgnoreImpCasts())) { + } else if (!isLiteralZero(BILC.getNonVariadicArg(1)->IgnoreImpCasts())) { S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 0; S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 0; } @@ -9767,17 +9745,16 @@ static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr *Call) { // 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(); + doesExprLikelyComputeSize(BILC.getNonVariadicArg(1)) && + !doesExprLikelyComputeSize(BILC.getNonVariadicArg(2))) { + SourceLocation DiagLoc = BILC.getNonVariadicArg(1)->getExprLoc(); S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 1; S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 1; return; } } -void Sema::CheckMemaccessArguments(const CallExpr *Call, - unsigned BId, +void Sema::CheckMemaccessArguments(const BuiltInLikeCall &BILC, unsigned BId, IdentifierInfo *FnName) { assert(BId != 0); @@ -9785,21 +9762,22 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, // we have enough arguments, and if not, abort further checking. unsigned ExpectedNumArgs = (BId == Builtin::BIstrndup || BId == Builtin::BIbzero ? 2 : 3); - if (Call->getNumArgs() < ExpectedNumArgs) + if (BILC.getCall()->getNumArgs() < ExpectedNumArgs) return; unsigned LastArg = (BId == Builtin::BImemset || BId == Builtin::BIbzero || BId == Builtin::BIstrndup ? 1 : 2); unsigned LenArg = (BId == Builtin::BIbzero || BId == Builtin::BIstrndup ? 1 : 2); - const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts(); + const Expr *LenExpr = BILC.getNonVariadicArg(LenArg)->IgnoreParenImpCasts(); if (CheckMemorySizeofForComparison(*this, LenExpr, FnName, - Call->getBeginLoc(), Call->getRParenLoc())) + BILC.getCall()->getBeginLoc(), + BILC.getCall()->getRParenLoc())) return; // Catch cases like 'memset(buf, sizeof(buf), 0)'. - CheckMemaccessSize(*this, BId, Call); + CheckMemaccessSize(*this, BId, BILC); // We have special checking when the length is a sizeof expression. QualType SizeOfArgTy = getSizeOfArgType(LenExpr); @@ -9809,13 +9787,15 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, // Although widely used, 'bzero' is not a standard function. Be more strict // with the argument types before allowing diagnostics and only allow the // form bzero(ptr, sizeof(...)). - QualType FirstArgTy = Call->getArg(0)->IgnoreParenImpCasts()->getType(); + QualType FirstArgTy = + BILC.getNonVariadicArg(0)->IgnoreParenImpCasts()->getType(); if (BId == Builtin::BIbzero && !FirstArgTy->getAs<PointerType>()) return; for (unsigned ArgIdx = 0; ArgIdx != LastArg; ++ArgIdx) { - const Expr *Dest = Call->getArg(ArgIdx)->IgnoreParenImpCasts(); - SourceRange ArgRange = Call->getArg(ArgIdx)->getSourceRange(); + const Expr *ArgAtIdx = BILC.getNonVariadicArg(ArgIdx); + const Expr *Dest = ArgAtIdx->IgnoreParenImpCasts(); + SourceRange ArgRange = ArgAtIdx->getSourceRange(); QualType DestTy = Dest->getType(); QualType PointeeTy; @@ -9929,14 +9909,13 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, PDiag(diag::warn_dyn_class_memaccess) << (IsCmp ? ArgIdx + 2 : ArgIdx) << FnName << IsContained << ContainedRD << OperationType - << Call->getCallee()->getSourceRange()); + << BILC.getCall()->getCallee()->getSourceRange()); } else if (PointeeTy.hasNonTrivialObjCLifetime() && BId != Builtin::BImemset) - DiagRuntimeBehavior( - Dest->getExprLoc(), Dest, - PDiag(diag::warn_arc_object_memaccess) - << ArgIdx << FnName << PointeeTy - << Call->getCallee()->getSourceRange()); + DiagRuntimeBehavior(Dest->getExprLoc(), Dest, + PDiag(diag::warn_arc_object_memaccess) + << ArgIdx << FnName << PointeeTy + << BILC.getCall()->getCallee()->getSourceRange()); else if (const auto *RT = PointeeTy->getAs<RecordType>()) { // FIXME: Do not consider incomplete types even though they may be @@ -10025,20 +10004,23 @@ static bool isConstantSizeArrayWithMoreThanOneElement(QualType Ty, return true; } -void Sema::CheckStrlcpycatArguments(const CallExpr *Call, +void Sema::CheckStrlcpycatArguments(const BuiltInLikeCall &BILC, IdentifierInfo *FnName) { // Don't crash if the user has the wrong number of arguments - unsigned NumArgs = Call->getNumArgs(); + unsigned NumArgs = BILC.getCall()->getNumArgs(); if ((NumArgs != 3) && (NumArgs != 4)) return; - const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context); - const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context); + const Expr *SrcArg = + ignoreLiteralAdditions(BILC.getNonVariadicArg(1), Context); + const Expr *SizeArg = + ignoreLiteralAdditions(BILC.getNonVariadicArg(2), Context); const Expr *CompareWithSrc = nullptr; if (CheckMemorySizeofForComparison(*this, SizeArg, FnName, - Call->getBeginLoc(), Call->getRParenLoc())) + BILC.getCall()->getBeginLoc(), + BILC.getCall()->getRParenLoc())) return; // Look for 'strlcpy(dst, x, sizeof(x))' @@ -10069,7 +10051,7 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call, SrcArgDRE->getDecl() != CompareWithSrcDRE->getDecl()) return; - const Expr *OriginalSizeArg = Call->getArg(2); + const Expr *OriginalSizeArg = BILC.getNonVariadicArg(2); Diag(CompareWithSrcDRE->getBeginLoc(), diag::warn_strlcpycat_wrong_size) << OriginalSizeArg->getSourceRange() << FnName; @@ -10077,7 +10059,7 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call, // pointer to an array). This could be enhanced to handle some // pointers if we know the actual size, like if DstArg is 'array+2' // we could say 'sizeof(array)-2'. - const Expr *DstArg = Call->getArg(0)->IgnoreParenImpCasts(); + const Expr *DstArg = BILC.getNonVariadicArg(0)->IgnoreParenImpCasts(); if (!isConstantSizeArrayWithMoreThanOneElement(DstArg->getType(), Context)) return; @@ -10110,17 +10092,18 @@ static const Expr *getStrlenExprArg(const Expr *E) { return nullptr; } -void Sema::CheckStrncatArguments(const CallExpr *CE, +void Sema::CheckStrncatArguments(const BuiltInLikeCall &BILC, const IdentifierInfo *FnName) { // Don't crash if the user has the wrong number of arguments. - if (CE->getNumArgs() < 3) + if (BILC.getCall()->getNumArgs() < 3) return; - const Expr *DstArg = CE->getArg(0)->IgnoreParenCasts(); - const Expr *SrcArg = CE->getArg(1)->IgnoreParenCasts(); - const Expr *LenArg = CE->getArg(2)->IgnoreParenCasts(); + const Expr *DstArg = BILC.getNonVariadicArg(0)->IgnoreParenCasts(); + const Expr *SrcArg = BILC.getNonVariadicArg(1)->IgnoreParenCasts(); + const Expr *LenArg = BILC.getNonVariadicArg(2)->IgnoreParenCasts(); - if (CheckMemorySizeofForComparison(*this, LenArg, FnName, CE->getBeginLoc(), - CE->getRParenLoc())) + if (CheckMemorySizeofForComparison(*this, LenArg, FnName, + BILC.getCall()->getBeginLoc(), + BILC.getCall()->getRParenLoc())) return; // Identify common expressions, which are wrongly used as the size argument @@ -10269,12 +10252,13 @@ void CheckFreeArgumentsCast(Sema &S, const std::string &CalleeName, } } // namespace -void Sema::CheckFreeArguments(const CallExpr *E) { +void Sema::CheckFreeArguments(const BuiltInLikeCall &BILC) { const std::string CalleeName = - cast<FunctionDecl>(E->getCalleeDecl())->getQualifiedNameAsString(); + cast<FunctionDecl>(BILC.getCall()->getCalleeDecl()) + ->getQualifiedNameAsString(); { // Prefer something that doesn't involve a cast to make things simpler. - const Expr *Arg = E->getArg(0)->IgnoreParenCasts(); + const Expr *Arg = BILC.getNonVariadicArg(0)->IgnoreParenCasts(); if (const auto *UnaryExpr = dyn_cast<UnaryOperator>(Arg)) switch (UnaryExpr->getOpcode()) { case UnaryOperator::Opcode::UO_AddrOf: @@ -10302,7 +10286,7 @@ void Sema::CheckFreeArguments(const CallExpr *E) { } } // Maybe the cast was important, check after the other cases. - if (const auto *Cast = dyn_cast<CastExpr>(E->getArg(0))) + if (const auto *Cast = dyn_cast<CastExpr>(BILC.getNonVariadicArg(0))) return CheckFreeArgumentsCast(*this, CalleeName, Cast); } diff --git a/clang/test/Sema/transpose-memset.c b/clang/test/Sema/transpose-memset.c index 7d83b8e336a08..321590df8c5e8 100644 --- a/clang/test/Sema/transpose-memset.c +++ b/clang/test/Sema/transpose-memset.c @@ -61,3 +61,12 @@ void macros(void) { __builtin_memset(array, 1, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}} __builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}} } + +// Custom memset with diagnose_as_builtin attribute +__attribute((diagnose_as_builtin(__builtin_memset, 2, 3, 1))) +void custom_memset(unsigned long num, void *ptr, int value); + +void dab(void) { + char cs[4]; + custom_memset(0, cs, 8); // 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}} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
