[clang][Sema] Don't warn for implicit uses of builtins in system headers #138205
Conversation
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nick Sarnie (sarnex) ChangesWhen trying to remove the usage of However, some MSVC headers already use the MSVC builtins without including the header, so we introduce a warning for anyone compiling with MSVC for this target, so the above change had to be reverted. As a workaround, don't warn for implicit uses of library functions if it's inside a system header, unless system header warnings are enabled. If this PR is accepted I will re-apply the original commit reworking the builtins. Full diff: https://github.com/llvm/llvm-project/pull/138205.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index a3285e8f6f5a2..37008f9eb3235 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2376,9 +2376,14 @@ NamedDecl *Sema::LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID,
return nullptr;
}
+ // Warn for implicit uses of header dependent libraries,
+ // except in system headers.
if (!ForRedeclaration &&
(Context.BuiltinInfo.isPredefinedLibFunction(ID) ||
- Context.BuiltinInfo.isHeaderDependentFunction(ID))) {
+ Context.BuiltinInfo.isHeaderDependentFunction(ID)) &&
+ (!getDiagnostics().getSuppressSystemWarnings() ||
+ !Context.getSourceManager().isInSystemHeader(
+ Context.getSourceManager().getSpellingLoc(Loc)))) {
Diag(Loc, LangOpts.C99 ? diag::ext_implicit_lib_function_decl_c99
: diag::ext_implicit_lib_function_decl)
<< Context.BuiltinInfo.getName(ID) << R;
diff --git a/clang/test/Sema/Inputs/builtin-system-header.h b/clang/test/Sema/Inputs/builtin-system-header.h
new file mode 100644
index 0000000000000..ebd5655e6f8ef
--- /dev/null
+++ b/clang/test/Sema/Inputs/builtin-system-header.h
@@ -0,0 +1 @@
+#define MACRO(x,y) _InterlockedOr64(x,y);
diff --git a/clang/test/Sema/builtin-system-header.c b/clang/test/Sema/builtin-system-header.c
new file mode 100644
index 0000000000000..83c3c15e314a7
--- /dev/null
+++ b/clang/test/Sema/builtin-system-header.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s
+
+// expected-no-diagnostics
+#include <builtin-system-header.h>
+
+void foo() {
+ MACRO(0,0);
+}
|
|
Something does not add up here. AFAICT, using builtins w/o explicitly declaring them is something that's done all the time. https://godbolt.org/z/ha47W53dh In that sense, we should not be needing to filter out the diagnostics coming from the system headers only. There should not be any diagnostics for those builtins at all, from anywhere. I must be missing something here. @rnk -- is the requirement for builtin declarations a windows-specific quirk? |
|
I think it's because we are explicitly marking these builtins as requiring a header. If they aren't defined that way in the Tablegen file then you won't see this warning and won't need this change. I made the builtins require a header based on feedback from @rnk in the earlier PR, and his suggestion on my regression fix PR to still require the header but follow what we did for So if I understand the state of affairs correctly if we want to prevent the
Let me know if anyone has suggestions, sorry this change is so drawn out :) |
|
OK. This makes sense.
What matters is that you're making progress, and I appreciate your work on getting this issue sorted out the right way. |
| Context.BuiltinInfo.isHeaderDependentFunction(ID)) && | ||
| (!getDiagnostics().getSuppressSystemWarnings() || | ||
| !Context.getSourceManager().isInSystemHeader( | ||
| Context.getSourceManager().getSpellingLoc(Loc)))) { |
There was a problem hiding this comment.
I think we may want to narrow down the scope of which diagnostics we want to suppress and when.
While we do know that we may need it for these builting for windows on ARM, I can not say whether we want to do that to other intrinsics for other targets.
That change was reverted for different reasons. I shouldn't have changed the prototype of So, llvm-project/clang/include/clang/Basic/BuiltinsX86.td Lines 4491 to 4498 in 50e1db7 Are you sure we need this extra change to suppress the warning? |
When trying to remove the usage of
__has_builtinon MSVC CUDA ARM for some builtins, the recommended direction was to universally declare the MSVC builtins on all platforms and require the header providing declarations to be included. This was done here.However, some MSVC headers already use the MSVC builtins without including the header, so we introduce a warning for anyone compiling with MSVC for this target, so the above change had to be reverted.
As a workaround, don't warn for implicit uses of library functions if it's inside a system header, unless system header warnings are enabled.
If this PR is accepted I will re-apply the original commit reworking the builtins.