[Clang] Fix to diagnose_as_builtin memset

[tynasello]

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. This fix adds the diagnosis for this case.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (tynasello)

Changes

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. This fix adds the diagnosis for this case.

---

Full diff: https://github.com/llvm/llvm-project/pull/154432.diff

5 Files Affected:

• (modified) clang/include/clang/AST/Expr.h (+26)
• (modified) clang/include/clang/Sema/Sema.h (+5-4)
• (modified) clang/lib/AST/Expr.cpp (+22)
• (modified) clang/lib/Sema/SemaChecking.cpp (+60-76)
• (modified) clang/test/Sema/transpose-memset.c (+9)

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}} +}