[clang] Catch missing format attributes #70024
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
Removing myself as a reviewer -- I don't really consider myself to have expertise here. |
aaronpuchert
added
clang:diagnostics
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
9d0b6ae
to
3647719
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Budimir Aranđelović (budimirarandjelovicsyrmia) ChangesEnable flag -Wmissing-format-attribute to catch missing attributes. Full diff: https://github.com/llvm/llvm-project/pull/70024.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 17fdcffa2d42740..b8b77df84beb2be 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -482,7 +482,7 @@ def MainReturnType : DiagGroup<"main-return-type">;
def MaxUnsignedZero : DiagGroup<"max-unsigned-zero">;
def MissingBraces : DiagGroup<"missing-braces">;
def MissingDeclarations: DiagGroup<"missing-declarations">;
-def : DiagGroup<"missing-format-attribute">;
+def MissingFormatAttribute: DiagGroup<"missing-format-attribute">;
def : DiagGroup<"missing-include-dirs">;
def MissingNoreturn : DiagGroup<"missing-noreturn">;
def MultiChar : DiagGroup<"multichar">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6d6f474f6dcdab9..7125de143c1710e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -936,6 +936,9 @@ def err_opencl_invalid_param : Error<
def err_opencl_invalid_return : Error<
"declaring function return value of type %0 is not allowed %select{; did you forget * ?|}1">;
def warn_enum_value_overflow : Warning<"overflow in enumeration value">;
+def warn_missing_format_attribute : Warning<
+ "Function %0 might be candidate for %1 format attribute.">,
+ InGroup<MissingFormatAttribute>, DefaultIgnore;
def warn_pragma_options_align_reset_failed : Warning<
"#pragma options align=reset failed: %0">,
InGroup<IgnoredPragmas>;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 741c2503127af7a..ee5ccfab7b82feb 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10615,6 +10615,9 @@ class Sema final {
ChangedStateAtExit
};
+ void DiagnoseMissingFormatAttributes(const FunctionDecl *FDecl,
+ SourceLocation Loc);
+
void DiagnoseNonDefaultPragmaAlignPack(PragmaAlignPackDiagnoseKind Kind,
SourceLocation IncludeLoc);
void DiagnoseUnterminatedPragmaAlignPack();
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index 42f582724564d06..ebd3e29a9efd507 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -435,6 +435,64 @@ bool Sema::ConstantFoldAttrArgs(const AttributeCommonInfo &CI,
return true;
}
+// Warn if parent function does not have builtin function format attribute.
+void Sema::DiagnoseMissingFormatAttributes(const FunctionDecl *FDecl,
+ SourceLocation Loc) {
+ assert(FDecl);
+
+ unsigned BuiltinID = FDecl->getBuiltinID(/*ConsiderWrappers=*/true);
+
+ // Function is not builtin if it's builtin ID is 0.
+ if (!BuiltinID)
+ return;
+
+ // Check if function is one with format attribute.
+ switch (BuiltinID) {
+ case Builtin::BIprintf:
+ case Builtin::BIfprintf:
+ case Builtin::BIsprintf:
+ case Builtin::BIscanf:
+ case Builtin::BIfscanf:
+ case Builtin::BIsscanf:
+ case Builtin::BIvprintf:
+ case Builtin::BIvfprintf:
+ case Builtin::BIvsprintf:
+ break;
+ // C99 mode
+ case Builtin::BIsnprintf:
+ case Builtin::BIvsnprintf:
+ case Builtin::BIvscanf:
+ case Builtin::BIvfscanf:
+ case Builtin::BIvsscanf:
+ if (!getLangOpts().C99)
+ return;
+ break;
+ default:
+ return;
+ }
+
+ const FunctionDecl *ParentFuncDecl = getCurFunctionDecl();
+ if (!ParentFuncDecl)
+ return;
+
+ // Iterate through builtin function format attributes. Then check
+ // if parent function has these attributes. If parent function does
+ // not have builtin function format attribut, emit warning.
+ for (const FormatAttr *Attr : FDecl->specific_attrs<FormatAttr>()) {
+ bool hasFormatAttr =
+ llvm::any_of(ParentFuncDecl->specific_attrs<FormatAttr>(),
+ [&](const FormatAttr *ParentAttr) {
+ // What about checking other arguments?
+ return ParentAttr->getType() == Attr->getType();
+ });
+ if (!hasFormatAttr) {
+ Diag(Loc, diag::warn_missing_format_attribute)
+ << ParentFuncDecl << Attr->getType();
+ // Diag(Loc, diag::warn_unannotated_fallthrough);
+ }
+ }
+}
+
void Sema::DiagnoseNonDefaultPragmaAlignPack(PragmaAlignPackDiagnoseKind Kind,
SourceLocation IncludeLoc) {
if (Kind == PragmaAlignPackDiagnoseKind::NonDefaultStateAtInclude) {
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 4602284309491c1..629354302dfd3a7 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -6014,8 +6014,10 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
}
}
- if (FD)
+ if (FD) {
diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc);
+ DiagnoseMissingFormatAttributes(FD, Range.getBegin());
+ }
}
/// CheckConstructorCall - Check a constructor call for correctness and safety
diff --git a/clang/test/Sema/attr-format-missing.c b/clang/test/Sema/attr-format-missing.c
new file mode 100644
index 000000000000000..1926d3d3163124b
--- /dev/null
+++ b/clang/test/Sema/attr-format-missing.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-format-attribute %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c99 -Wmissing-format-attribute %s
+
+#include <stdarg.h>
+#include <stdio.h>
+
+__attribute__((__format__ (__scanf__, 1, 4)))
+void foo(char *out, const size_t len, const char *format, ... /* args */)
+{
+ va_list args;
+
+ vsnprintf(out, len, format, args); // expected-warning {{Function 'foo' might be candidate for 'printf' format attribute}}
+}
|
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
2 times, most recently
from
a70de33
to
845235e
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
845235e
to
9d49cf5
Compare
AaronBallman
reviewed
Dec 6, 2023
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
3 times, most recently
from
002bc02
to
795a2db
Compare
There was a problem hiding this comment.
Two additional checks that might be interesting:
- Look at the
FormatIdxargument. Is it aDeclRefExprreferring to aParmVarDecl, perhaps with some conversions? (There is probably going to be anLValueToRValueconversion at least. There are specialIgnore*functions onExprto unwrap that.) If it is, we should propagate the attribute, if not then not. - For
FirstArgit seems that GCC only warns when we call av*function, or the called function hasFirstArg == 0, probably because forwarding to a variadic function isn't really possible in C. It doesn't seem to check whether we're actually forwarding. What you've implemented here (deciding based onParentFuncDecl->isVariadic()) seems like a reasonable heuristic. But it probably makes sense to additionally restrict based on the archetype orFirstArg. Or is that what you're doing withArgs[Attr->getFirstArg()]?
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
795a2db
to
c8d3fbd
Compare
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
c8d3fbd
to
ca3bfcd
Compare
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
ca3bfcd
to
7adedf5
Compare
|
I'm not able to review Sema.h changes because of a merge conflict. |
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
2 times, most recently
from
30d1b50
to
2abeecc
Compare
|
Pre-commit CI seems to have found a valid failure: |
budimirarandjelovichtec
force-pushed
the
missing-format-attribute
branch
from
2abeecc
to
569ecf2
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/2140 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/1528 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/1303 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/1294 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/1409 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/1226 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/972 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/1123 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/686 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/925 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/896 Here is the relevant piece of the build log for the reference: |
AaronBallman
added a commit
that referenced
this pull request
Jul 12, 2024
This reverts commit 70f57d2.
AaronBallman
added a commit
that referenced
this pull request
Jul 12, 2024
Reverts #70024 It broke several post-commit bots: https://lab.llvm.org/buildbot/#/builders/193/builds/896 https://lab.llvm.org/buildbot/#/builders/23/builds/925 https://lab.llvm.org/buildbot/#/builders/13/builds/686 and others
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/920 Here is the relevant piece of the build log for the reference: |
|
Problem is in function f41 where diagnosing depends on architecture and operating system. As this is unusual situation, I suppose to remove this function from test. |
|
There are multiple distinct issues that were caught: build failure -- https://lab.llvm.org/buildbot/#/builders/72/builds/1123 You should go through each of the failures individually to ensure you've addressed all the concerns. I would not remove test coverage, but instead repair the tests. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/449 Here is the relevant piece of the build log for the reference: |
|
@budimirarandjelovicsyrmia Any updates? Should we revert the patch? |
I already reverted in 9d10172 so the bots have been slowly getting back to green |
|
Thank you! |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
Enable flag -Wmissing-format-attribute to catch missing attributes. Fixes llvm#60718
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
Reverts llvm#70024 It broke several post-commit bots: https://lab.llvm.org/buildbot/#/builders/193/builds/896 https://lab.llvm.org/buildbot/#/builders/23/builds/925 https://lab.llvm.org/buildbot/#/builders/13/builds/686 and others
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable flag -Wmissing-format-attribute to catch missing attributes.
Fixes #60718