[Sema] Suggest missing format attributes #166738
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| #include "clang/AST/ASTDiagnostic.h" | ||
| #include "clang/AST/Attr.h" | ||
| #include "clang/AST/AttrIterator.h" | ||
| #include "clang/AST/Attrs.inc" | ||
| #include "clang/AST/CharUnits.h" | ||
| #include "clang/AST/Decl.h" | ||
| #include "clang/AST/DeclBase.h" | ||
|
|
@@ -40,6 +41,7 @@ | |
| #include "clang/AST/TypeLoc.h" | ||
| #include "clang/AST/UnresolvedSet.h" | ||
| #include "clang/Basic/AddressSpaces.h" | ||
| #include "clang/Basic/CharInfo.h" | ||
| #include "clang/Basic/Diagnostic.h" | ||
| #include "clang/Basic/DiagnosticSema.h" | ||
| #include "clang/Basic/IdentifierTable.h" | ||
|
|
@@ -6511,13 +6513,16 @@ static const Expr *maybeConstEvalStringLiteral(ASTContext &Context, | |
| // If this function returns false on the arguments to a function expecting a | ||
| // format string, we will usually need to emit a warning. | ||
| // True string literals are then checked by CheckFormatString. | ||
| static StringLiteralCheckType | ||
| checkFormatStringExpr(Sema &S, const StringLiteral *ReferenceFormatString, | ||
| const Expr *E, ArrayRef<const Expr *> Args, | ||
| Sema::FormatArgumentPassingKind APK, unsigned format_idx, | ||
| unsigned firstDataArg, FormatStringType Type, | ||
| VariadicCallType CallType, bool InFunctionCall, | ||
| llvm::SmallBitVector &CheckedVarArgs, | ||
| UncoveredArgHandler &UncoveredArg, llvm::APSInt Offset, | ||
| std::optional<unsigned> *CallerFormatParamIdx = nullptr, | ||
| bool IgnoreStringsWithoutSpecifiers = false) { | ||
| if (S.isConstantEvaluatedContext()) | ||
| return SLCT_NotALiteral; | ||
| tryAgain: | ||
|
|
@@ -6539,10 +6544,11 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| case Stmt::InitListExprClass: | ||
| // Handle expressions like {"foobar"}. | ||
| if (const clang::Expr *SLE = maybeConstEvalStringLiteral(S.Context, E)) { | ||
| return checkFormatStringExpr(S, ReferenceFormatString, SLE, Args, APK, | ||
| format_idx, firstDataArg, Type, CallType, | ||
| /*InFunctionCall*/ false, CheckedVarArgs, | ||
| UncoveredArg, Offset, CallerFormatParamIdx, | ||
| IgnoreStringsWithoutSpecifiers); | ||
| } | ||
| return SLCT_NotALiteral; | ||
| case Stmt::BinaryConditionalOperatorClass: | ||
|
|
@@ -6574,10 +6580,11 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| if (!CheckLeft) | ||
| Left = SLCT_UncheckedLiteral; | ||
| else { | ||
| Left = checkFormatStringExpr(S, ReferenceFormatString, C->getTrueExpr(), | ||
| Args, APK, format_idx, firstDataArg, Type, | ||
| CallType, InFunctionCall, CheckedVarArgs, | ||
| UncoveredArg, Offset, CallerFormatParamIdx, | ||
| IgnoreStringsWithoutSpecifiers); | ||
| if (Left == SLCT_NotALiteral || !CheckRight) { | ||
| return Left; | ||
| } | ||
|
|
@@ -6586,7 +6593,8 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| StringLiteralCheckType Right = checkFormatStringExpr( | ||
| S, ReferenceFormatString, C->getFalseExpr(), Args, APK, format_idx, | ||
| firstDataArg, Type, CallType, InFunctionCall, CheckedVarArgs, | ||
| UncoveredArg, Offset, CallerFormatParamIdx, | ||
| IgnoreStringsWithoutSpecifiers); | ||
|
|
||
| return (CheckLeft && Left < Right) ? Left : Right; | ||
| } | ||
|
|
@@ -6637,8 +6645,8 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| } | ||
| return checkFormatStringExpr( | ||
| S, ReferenceFormatString, Init, Args, APK, format_idx, | ||
| firstDataArg, Type, CallType, /*InFunctionCall=*/false, | ||
| CheckedVarArgs, UncoveredArg, Offset, CallerFormatParamIdx); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -6690,6 +6698,8 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| // format arguments, in all cases. | ||
| // | ||
| if (const auto *PV = dyn_cast<ParmVarDecl>(VD)) { | ||
| if (CallerFormatParamIdx) | ||
| *CallerFormatParamIdx = PV->getFunctionScopeIndex(); | ||
| if (const auto *D = dyn_cast<Decl>(PV->getDeclContext())) { | ||
| for (const auto *PVFormatMatches : | ||
| D->specific_attrs<FormatMatchesAttr>()) { | ||
|
|
@@ -6715,7 +6725,7 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| S, ReferenceFormatString, PVFormatMatches->getFormatString(), | ||
| Args, APK, format_idx, firstDataArg, Type, CallType, | ||
| /*InFunctionCall*/ false, CheckedVarArgs, UncoveredArg, | ||
| Offset, CallerFormatParamIdx, IgnoreStringsWithoutSpecifiers); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -6770,7 +6780,7 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| StringLiteralCheckType Result = checkFormatStringExpr( | ||
| S, ReferenceFormatString, Arg, Args, APK, format_idx, firstDataArg, | ||
| Type, CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, | ||
| Offset, CallerFormatParamIdx, IgnoreStringsWithoutSpecifiers); | ||
| if (IsFirst) { | ||
| CommonResult = Result; | ||
| IsFirst = false; | ||
|
|
@@ -6787,15 +6797,17 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| return checkFormatStringExpr( | ||
| S, ReferenceFormatString, Arg, Args, APK, format_idx, | ||
| firstDataArg, Type, CallType, InFunctionCall, CheckedVarArgs, | ||
| UncoveredArg, Offset, CallerFormatParamIdx, | ||
| IgnoreStringsWithoutSpecifiers); | ||
| } | ||
| } | ||
| } | ||
| if (const Expr *SLE = maybeConstEvalStringLiteral(S.Context, E)) | ||
| return checkFormatStringExpr(S, ReferenceFormatString, SLE, Args, APK, | ||
| format_idx, firstDataArg, Type, CallType, | ||
| /*InFunctionCall*/ false, CheckedVarArgs, | ||
| UncoveredArg, Offset, CallerFormatParamIdx, | ||
| IgnoreStringsWithoutSpecifiers); | ||
| return SLCT_NotALiteral; | ||
| } | ||
| case Stmt::ObjCMessageExprClass: { | ||
|
|
@@ -6821,7 +6833,7 @@ static StringLiteralCheckType checkFormatStringExpr( | |
| return checkFormatStringExpr( | ||
| S, ReferenceFormatString, Arg, Args, APK, format_idx, firstDataArg, | ||
| Type, CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, | ||
| Offset, CallerFormatParamIdx, IgnoreStringsWithoutSpecifiers); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -7001,9 +7013,133 @@ bool Sema::CheckFormatString(const FormatMatchesAttr *Format, | |
| return false; | ||
| } | ||
|
|
||
| std::string escapeFormatString(StringRef Input) { | ||
| std::string Result; | ||
| llvm::raw_string_ostream OS(Result); | ||
|
Contributor
There was a problem hiding this comment. This is fine, but OS.write_escaped does something pretty similar to this loop. |
||
| for (char C : Input) { | ||
| StringRef Escaped = escapeCStyle<EscapeChar::Double>(C); | ||
| if (Escaped.empty()) | ||
| OS << C; | ||
| else | ||
| OS << Escaped; | ||
| } | ||
| return Result; | ||
| } | ||
|
|
||
| static void CheckMissingFormatAttributes( | ||
| Sema *S, ArrayRef<const Expr *> Args, Sema::FormatArgumentPassingKind APK, | ||
| StringLiteral *ReferenceFormatString, unsigned FormatIdx, | ||
| unsigned FirstDataArg, FormatStringType FormatType, unsigned CallerParamIdx, | ||
| SourceLocation Loc) { | ||
| NamedDecl *Caller = S->getCurFunctionOrMethodDecl(); | ||
| if (!Caller) | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. This will always fail in Objective-C because ObjCMethodDecl does not inherit from FunctionDecl. Would you add an Objective-C test to ensure this works? If you're not super familiar with it, the declaration/use syntax should be: Happy to answer other questions you may have.
Contributor
There was a problem hiding this comment. I think this also always fails for blocks, but it matters less because
Contributor
Author
There was a problem hiding this comment. I'm not familiar with objective-c and there doesn't seem to be much documentation regarding attributes, but I think I have a working implementation. Just one question: is there an implicit 'this' argument on obj-c methods? I'll have to look into blocks to see how they fit in here.
Contributor
There was a problem hiding this comment. Objective-C methods have two implicit arguments (
Contributor
Author
There was a problem hiding this comment. Added support for Objective-C. Let me know if there are other cases I should test. Also, I wasn't sure which attribute syntax is supported, so I used the GNU extension.
Contributor
There was a problem hiding this comment. Seems to cover it, thank you!
Contributor
There was a problem hiding this comment. I tested where Objective-C tolerates attributes, and the fixit is incorrect currently: #include <stdarg.h>
__attribute__((format(printf, 1, 0)))
int vprintf(const char *, va_list ap);
@interface Foo
-(void)printf:(const char *)fmt, ...;
@end
@implementation Foo
-(void)printf:(const char *)fmt, ... {
va_list ap;
va_start(ap, fmt);
vprintf(fmt, ap);
va_end(ap);
}
@endThe fixit will suggest To make it as effective as possible, the fixit should also go to the canonical declaration, as returned by Here's a patch for your change which I think does the trick (but that I didn't test extensively): objc-placement.patch |
||
| Caller = dyn_cast<NamedDecl>(Caller->getCanonicalDecl()); | ||
|
|
||
| unsigned NumCallerParams = getFunctionOrMethodNumParams(Caller); | ||
|
|
||
| // Find the offset to convert between attribute and parameter indexes. | ||
| unsigned CallerArgumentIndexOffset = | ||
| hasImplicitObjectParameter(Caller) ? 2 : 1; | ||
|
|
||
| unsigned FirstArgumentIndex = -1; | ||
| switch (APK) { | ||
| case Sema::FormatArgumentPassingKind::FAPK_Fixed: | ||
| case Sema::FormatArgumentPassingKind::FAPK_Variadic: { | ||
| // As an extension, clang allows the format attribute on non-variadic | ||
| // functions. | ||
| // Caller must have fixed arguments to pass them to a fixed or variadic | ||
| // function. Try to match caller and callee arguments. If successful, then | ||
| // emit a diag with the caller idx, otherwise we can't determine the callee | ||
| // arguments. | ||
| unsigned NumCalleeArgs = Args.size() - FirstDataArg; | ||
| if (NumCalleeArgs == 0 || NumCallerParams < NumCalleeArgs) { | ||
| // There aren't enough arguments in the caller to pass to callee. | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. It's possible in this case that one could use
Contributor
Author
There was a problem hiding this comment. I'd have to generate a format string for |
||
| } | ||
| for (unsigned CalleeIdx = Args.size() - 1, CallerIdx = NumCallerParams - 1; | ||
| CalleeIdx >= FirstDataArg; --CalleeIdx, --CallerIdx) { | ||
| const auto *Arg = | ||
| dyn_cast<DeclRefExpr>(Args[CalleeIdx]->IgnoreParenCasts()); | ||
| if (!Arg) | ||
| return; | ||
| const auto *Param = dyn_cast<ParmVarDecl>(Arg->getDecl()); | ||
| if (!Param || Param->getFunctionScopeIndex() != CallerIdx) | ||
| return; | ||
| } | ||
| FirstArgumentIndex = | ||
| NumCallerParams + CallerArgumentIndexOffset - NumCalleeArgs; | ||
| break; | ||
| } | ||
| case Sema::FormatArgumentPassingKind::FAPK_VAList: | ||
| // Caller arguments are either variadic or a va_list. | ||
| FirstArgumentIndex = isFunctionOrMethodVariadic(Caller) | ||
| ? (NumCallerParams + CallerArgumentIndexOffset) | ||
| : 0; | ||
| break; | ||
| case Sema::FormatArgumentPassingKind::FAPK_Elsewhere: | ||
| // The callee has a format_matches attribute. We will emit that instead. | ||
| if (!ReferenceFormatString) | ||
| return; | ||
| break; | ||
| } | ||
|
|
||
| // Emit the diagnostic and fixit. | ||
|
Contributor
There was a problem hiding this comment. As a recovery action, should we also add an implicit format attribute to
Contributor
Author
There was a problem hiding this comment. That changes some behavior: void foo(const char *fmt, va_list args) {
vprintf(fmt, args);
vscanf(fmt, args);
}Previously, this would emit two diagnostics to suggest a printf and scanf format attribute. If we add an implicit attr, then when looking at I'll see if there are any other differences before updating the PR, but these changes make sense to me.
Contributor
There was a problem hiding this comment. These changes also make sense to me.
Contributor
There was a problem hiding this comment. Oh, one thing to mention is that we should only add the attribute if the diagnostic is enabled at the call site, otherwise people will start seeing inexplicable format warnings that cannot be silenced. This would probably be a net positive, but the respectful way to do this would be to enable the diagnostic by default. |
||
| unsigned FormatStringIndex = CallerParamIdx + CallerArgumentIndexOffset; | ||
| StringRef FormatTypeName = S->GetFormatStringTypeName(FormatType); | ||
| do { | ||
| std::string Attr, Fixit; | ||
| if (APK != Sema::FormatArgumentPassingKind::FAPK_Elsewhere) | ||
| llvm::raw_string_ostream(Attr) | ||
| << "format(" << FormatTypeName << ", " << FormatStringIndex << ", " | ||
| << FirstArgumentIndex << ")"; | ||
| else | ||
| llvm::raw_string_ostream(Attr) | ||
| << "format_matches(" << FormatTypeName << ", " << FormatStringIndex | ||
| << ", \"" << escapeFormatString(ReferenceFormatString->getString()) | ||
| << "\")"; | ||
| auto DB = S->Diag(Loc, diag::warn_missing_format_attribute) | ||
| << Attr << Caller; | ||
|
|
||
| SourceLocation SL; | ||
| llvm::raw_string_ostream IS(Fixit); | ||
| // The attribute goes at the start of the declaration in C/C++ functions | ||
| // and methods, but after the declaration for Objective-C methods. | ||
| if (isa<ObjCMethodDecl>(Caller)) { | ||
| IS << ' '; | ||
| SL = Caller->getEndLoc(); | ||
| } | ||
| const LangOptions &LO = S->getLangOpts(); | ||
| if (LO.C23 || LO.CPlusPlus11) | ||
| IS << "[[gnu::" << Attr << "]]"; | ||
| else if (LO.ObjC || LO.GNUMode) | ||
| IS << "__attribute__((" << Attr << "))"; | ||
| else | ||
| break; | ||
| if (!isa<ObjCMethodDecl>(Caller)) { | ||
| IS << ' '; | ||
| SL = Caller->getBeginLoc(); | ||
| } | ||
| IS.flush(); | ||
|
|
||
| DB << FixItHint::CreateInsertion(SL, Fixit); | ||
| } while (false); | ||
| S->Diag(Caller->getLocation(), diag::note_entity_declared_at) << Caller; | ||
|
|
||
| if (APK != Sema::FormatArgumentPassingKind::FAPK_Elsewhere) { | ||
|
Contributor
There was a problem hiding this comment. We should only do this if |
||
| Caller->addAttr(FormatAttr::CreateImplicit( | ||
| S->getASTContext(), &S->getASTContext().Idents.get(FormatTypeName), | ||
| FormatStringIndex, FirstArgumentIndex)); | ||
| } else { | ||
| Caller->addAttr(FormatMatchesAttr::CreateImplicit( | ||
| S->getASTContext(), &S->getASTContext().Idents.get(FormatTypeName), | ||
| FormatStringIndex, ReferenceFormatString)); | ||
| } | ||
| } | ||
|
|
||
| bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args, | ||
| Sema::FormatArgumentPassingKind APK, | ||
| StringLiteral *ReferenceFormatString, | ||
| unsigned format_idx, unsigned firstDataArg, | ||
| FormatStringType Type, | ||
| VariadicCallType CallType, SourceLocation Loc, | ||
|
|
@@ -7030,11 +7166,12 @@ bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args, | |
| // ObjC string uses the same format specifiers as C string, so we can use | ||
| // the same format string checking logic for both ObjC and C strings. | ||
| UncoveredArgHandler UncoveredArg; | ||
| std::optional<unsigned> CallerParamIdx; | ||
| StringLiteralCheckType CT = checkFormatStringExpr( | ||
| *this, ReferenceFormatString, OrigFormatExpr, Args, APK, format_idx, | ||
| firstDataArg, Type, CallType, | ||
| /*IsFunctionCall*/ true, CheckedVarArgs, UncoveredArg, | ||
| /*no string offset*/ llvm::APSInt(64, false) = 0, &CallerParamIdx); | ||
|
|
||
| // Generate a diagnostic where an uncovered argument is detected. | ||
| if (UncoveredArg.hasUncoveredArg()) { | ||
|
|
@@ -7047,11 +7184,6 @@ bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args, | |
| // Literal format string found, check done! | ||
| return CT == SLCT_CheckedLiteral; | ||
|
|
||
| // Do not emit diag when the string param is a macro expansion and the | ||
| // format is either NSString or CFString. This is a hack to prevent | ||
| // diag when using the NSLocalizedString and CFCopyLocalizedString macros | ||
|
|
@@ -7061,6 +7193,17 @@ bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args, | |
| SourceMgr.isInSystemMacro(FormatLoc)) | ||
| return false; | ||
|
|
||
| const LangOptions &LO = getLangOpts(); | ||
| if (CallerParamIdx && (LO.GNUMode || LO.C23 || LO.CPlusPlus11)) | ||
|
Contributor
There was a problem hiding this comment. You should warn in all cases and use the language mode only to decide whether to offer a fixit.
Member
There was a problem hiding this comment. The issue I see w/ that is that for language modes that don’t support
Contributor
Author
There was a problem hiding this comment.
I'm not sure if the warning that suggests adding an attribute makes sense if there isn't a portable way to actually add the attribute.
Contributor
There was a problem hiding this comment. There are portable ways to annotate your functions, it's just that you can't offer them as fixits for a variety of reasons (for instance, create a macro that adds the format attribute based on a But even then, I disagree that the only value (or even the main value) of any diagnostic is the fixit. Clang doesn't only diagnose situations that it knows how to fix. For a codebase that truly can't do anything about missing format attributes, the solution is to not take the intentional step of enabling the diagnostic.
Contributor
Author
There was a problem hiding this comment. Right, the user can figure out a way to write the attribute in a suitable way. I have changed the diagnostic message to include the attribute parameters as well like: so even if there is no fixit, the full suggested attribute is still shown. |
||
| CheckMissingFormatAttributes(this, Args, APK, ReferenceFormatString, | ||
| format_idx, firstDataArg, Type, | ||
| *CallerParamIdx, Loc); | ||
|
|
||
| // Strftime is particular as it always uses a single 'time' argument, | ||
| // so it is safe to pass a non-literal string. | ||
| if (Type == FormatStringType::Strftime) | ||
| return false; | ||
|
|
||
| // If there are no arguments specified, warn with -Wformat-security, otherwise | ||
| // warn only with -Wformat-nonliteral. | ||
| if (Args.size() == firstDataArg) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#60718 seems to be closed already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That issue was closed by #70024, but it was later reverted. Then #105479 was created to fix the issues, but never merged.
This is an updated version of #105479 with most comments addressed.