[clang][Sema] Declare builtins used in #pragma intrinsic #138205
Conversation
Signed-off-by: Sarnie, Nick <[email protected]>
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. |
clang/lib/Sema/SemaDecl.cpp
Outdated
| 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? |
Thanks, my bad, I should have looked at the problem in more detail.
You're right, we already have a check here to not throw diagnostics with source locations in system headers. The problem here is the source location is not in the system header, it's the user's call of the system header/builtin function (see repro below). Only the "Spelling" (whatever that means) part of the diagnostic is in the header, and the check doesn't check for that. Also, it looks like we don't see this problem with also if I make a declaration for So to summarize, it seems the root issue is that MSVC headers don't include a declaration for Maybe that justifies special casing this warning? Repro (from here) Of course you'll need to revert b60ee39 first. |
|
@rnk Any feedback on my above comment? Hopefully I can merge a solution to the issue soon. Thanks! |
|
Sorry for the delay! I opened up winnt.h and I see this line, which is supposed to make the We have a handler for that pragma, but it doesn't declare the builtins: I think the bug is that it should. Clearly mentioning it this way is enough to make the builtin available for MSVC, so it would make clang-cl more compatible if we did as well. I see that |
Signed-off-by: Sarnie, Nick <[email protected]>
clang/include/clang/Sema/Sema.h
Outdated
|
|
||
| /// Map of BuiltinIDs to source locations that have #pragma intrinsic calls | ||
| /// that refer to them. | ||
| llvm::DenseMap<unsigned, llvm::SmallSetVector<SourceLocation, 4>> |
There was a problem hiding this comment.
DenseMap of SmallVector values is not an efficient data structure choice, because the DenseMap is always at least 25% empty, and each SmallVector is likely to contain exactly 1 element.
I think we need to serialize these pragmas through the AST. Is it possible to do this by creating an implicit declaration of the builtin at the point of the pragma? You would test for this by making your system header into a module or PCH file in another test case.
There was a problem hiding this comment.
Sorry, I intended to write a comment explaining this commit but I got distracted and you reviewed it too fast :)
I tried creating the declaration just by calling LazilyCreateBuiltin in the pragma handler, but that caused problems when there is a declaration in the header because then we have two real declarations so we get a ambiguous call error. The reason that doesn't happen today with no changes is because we only call LazilyCreateBuiltin when the lookup fails, as in when there is no header providing a declaration and the symbol is totally unknown.
Let me try your idea, thanks for the feedback.
There was a problem hiding this comment.
I tried compiling the system header into a PCH and dumping the AST with llvm-bcanalyzer and I didn't see any implicit declarations, not sure if I was checking for the right thing.
Either way, I tried another solution where we declare the builtin if it wasn't already declared. Fixes the ambiguous call errors I saw when trying to universally declare it and fixes the original issue. Please take a look when you have a sec, thanks.
Signed-off-by: Sarnie, Nick <[email protected]>
|
Thanks for the guidance and reviews! |
sarnex
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/18115 Here is the relevant piece of the build log for the reference |
|
Chrome is seeing some breakages due to this: https://crbug.com/420757016 |
|
Investigating, thanks. |
|
It took me all day but I managed to reproduce the issue with Chromium, investigating more. |
|
@alanzhao1 let me know if this is a big problem for Chrome and I can revert it, I'd prefer to try to fix it quickly first and if I can't then revert it, thanks. |
|
Chrome requested I revert |
…rinsic" (#141994) Reverts llvm/llvm-project#138205 Breaks Chrome.
Thanks for the revert! I kind of requested that more with my clang hat on than my chrome hat – we try to keep trunk green as much as possible, and since you have a repro, reverting first makes trunk green sooner, and removes time pressure when investigating the problem :) |
|
Sure, I was hoping I could find an easy fix really quickly but just reproing took me a while, I'll revert immediately in the future, thanks. |
…insic #138205' (#142019) I had to revert #138205 in #141994 because it broke the Chrome build. The problem came down to the following: ```c++ unsigned __int64 _umul128(unsigned __int64, unsigned __int64, unsigned __int64 *); namespace {} #pragma intrinsic(_umul128) void foo() { unsigned __int64 carry; unsigned __int64 low = _umul128(0, 0, &carry); } ``` When processing the `#pragma intrinsic` line, we do a name lookup to see if the builtin was previously declared. In this case the lookup fails because the current namespace of the parser and sema is the above namespace scope. The processing of the pragma happens as part of the namespace close parsing. This is usually fine because most pragmas don't care about scopes. However, that's not true for this and other MS pragmas. To fix this, we change the `#pragma intrinsic` processing to be the same as other MS pragmas such as "optimize". Those are processed like a declaration, and because of that we have the correct current scope, so the lookup succeeds. I added a test case that locks down the Chrome fix, as well as manually tested the Chrome build and confirmed it passed.
…ragma intrinsic #138205' (#142019) I had to revert llvm/llvm-project#138205 in llvm/llvm-project#141994 because it broke the Chrome build. The problem came down to the following: ```c++ unsigned __int64 _umul128(unsigned __int64, unsigned __int64, unsigned __int64 *); namespace {} #pragma intrinsic(_umul128) void foo() { unsigned __int64 carry; unsigned __int64 low = _umul128(0, 0, &carry); } ``` When processing the `#pragma intrinsic` line, we do a name lookup to see if the builtin was previously declared. In this case the lookup fails because the current namespace of the parser and sema is the above namespace scope. The processing of the pragma happens as part of the namespace close parsing. This is usually fine because most pragmas don't care about scopes. However, that's not true for this and other MS pragmas. To fix this, we change the `#pragma intrinsic` processing to be the same as other MS pragmas such as "optimize". Those are processed like a declaration, and because of that we have the correct current scope, so the lookup succeeds. I added a test case that locks down the Chrome fix, as well as manually tested the Chrome build and confirmed it passed.
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.
The MSVC headers use
#pragma intrinsicbefore the intrinsic uses and that seems to be enough for MSVC, so declare builtins when used in#pragma intrinsicin Clang to prevent the warning.