[clang][Sema] Fix and reapply 'Declare builtins used in #pragma intrinsic #138205'

[sarnex]

I had to revert #138205 in #141994 because it broke the Chrome build.

The problem came down to the following:

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.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nick Sarnie (sarnex)

Changes

I had to revert #138205 in #138205 because it broke the Chrome build.

The problem came down to the following:

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.

---

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

5 Files Affected:

• (modified) clang/include/clang/Parse/Parser.h (+4)
• (modified) clang/lib/Parse/ParsePragma.cpp (+53-58)
• (added) clang/test/Sema/Inputs/builtin-system-header.h (+9)
• (added) clang/test/Sema/builtin-pragma-intrinsic-namespace.cpp (+32)
• (added) clang/test/Sema/builtin-pragma-intrinsic.c (+25)

diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index c4bef4729fd36..98db8201390be 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -7074,6 +7074,10 @@ class Parser : public CodeCompletionHandler {
   bool HandlePragmaMSOptimize(StringRef PragmaName,
                               SourceLocation PragmaLocation);
 
+  // #pragma intrinsic("foo")
+  bool HandlePragmaMSIntrinsic(StringRef PragmaName,
+                               SourceLocation PragmaLocation);
+
   /// Handle the annotation token produced for
   /// #pragma align...
   void HandlePragmaAlign();
diff --git a/clang/lib/Parse/ParsePragma.cpp b/clang/lib/Parse/ParsePragma.cpp
index 77b61af768993..6341e565b5042 100644
--- a/clang/lib/Parse/ParsePragma.cpp
+++ b/clang/lib/Parse/ParsePragma.cpp
@@ -300,12 +300,6 @@ struct PragmaMSRuntimeChecksHandler : public EmptyPragmaHandler {
   PragmaMSRuntimeChecksHandler() : EmptyPragmaHandler("runtime_checks") {}
 };
 
-struct PragmaMSIntrinsicHandler : public PragmaHandler {
-  PragmaMSIntrinsicHandler() : PragmaHandler("intrinsic") {}
-  void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
-                    Token &FirstToken) override;
-};
-
 // "\#pragma fenv_access (on)".
 struct PragmaMSFenvAccessHandler : public PragmaHandler {
   PragmaMSFenvAccessHandler() : PragmaHandler("fenv_access") {}
@@ -517,7 +511,7 @@ void Parser::initializePragmaHandlers() {
     PP.AddPragmaHandler(MSOptimize.get());
     MSRuntimeChecks = std::make_unique<PragmaMSRuntimeChecksHandler>();
     PP.AddPragmaHandler(MSRuntimeChecks.get());
-    MSIntrinsic = std::make_unique<PragmaMSIntrinsicHandler>();
+    MSIntrinsic = std::make_unique<PragmaMSPragma>("intrinsic");
     PP.AddPragmaHandler(MSIntrinsic.get());
     MSFenvAccess = std::make_unique<PragmaMSFenvAccessHandler>();
     PP.AddPragmaHandler(MSFenvAccess.get());
@@ -1046,7 +1040,8 @@ void Parser::HandlePragmaMSPragma() {
           .Case("strict_gs_check", &Parser::HandlePragmaMSStrictGuardStackCheck)
           .Case("function", &Parser::HandlePragmaMSFunction)
           .Case("alloc_text", &Parser::HandlePragmaMSAllocText)
-          .Case("optimize", &Parser::HandlePragmaMSOptimize);
+          .Case("optimize", &Parser::HandlePragmaMSOptimize)
+          .Case("intrinsic", &Parser::HandlePragmaMSIntrinsic);
 
   if (!(this->*Handler)(PragmaName, PragmaLocation)) {
     // Pragma handling failed, and has been diagnosed.  Slurp up the tokens
@@ -3762,56 +3757,6 @@ void PragmaUnrollHintHandler::HandlePragma(Preprocessor &PP,
                       /*DisableMacroExpansion=*/false, /*IsReinject=*/false);
 }
 
-/// Handle the Microsoft \#pragma intrinsic extension.
-///
-/// The syntax is:
-/// \code
-///  #pragma intrinsic(memset)
-///  #pragma intrinsic(strlen, memcpy)
-/// \endcode
-///
-/// Pragma intrisic tells the compiler to use a builtin version of the
-/// function. Clang does it anyway, so the pragma doesn't really do anything.
-/// Anyway, we emit a warning if the function specified in \#pragma intrinsic
-/// isn't an intrinsic in clang and suggest to include intrin.h.
-void PragmaMSIntrinsicHandler::HandlePragma(Preprocessor &PP,
-                                            PragmaIntroducer Introducer,
-                                            Token &Tok) {
-  PP.Lex(Tok);
-
-  if (Tok.isNot(tok::l_paren)) {
-    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen)
-        << "intrinsic";
-    return;
-  }
-  PP.Lex(Tok);
-
-  bool SuggestIntrinH = !PP.isMacroDefined("__INTRIN_H");
-
-  while (Tok.is(tok::identifier)) {
-    IdentifierInfo *II = Tok.getIdentifierInfo();
-    if (!II->getBuiltinID())
-      PP.Diag(Tok.getLocation(), diag::warn_pragma_intrinsic_builtin)
-          << II << SuggestIntrinH;
-
-    PP.Lex(Tok);
-    if (Tok.isNot(tok::comma))
-      break;
-    PP.Lex(Tok);
-  }
-
-  if (Tok.isNot(tok::r_paren)) {
-    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_rparen)
-        << "intrinsic";
-    return;
-  }
-  PP.Lex(Tok);
-
-  if (Tok.isNot(tok::eod))
-    PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
-        << "intrinsic";
-}
-
 bool Parser::HandlePragmaMSFunction(StringRef PragmaName,
                                     SourceLocation PragmaLocation) {
   Token FirstTok = Tok;
@@ -3907,6 +3852,56 @@ bool Parser::HandlePragmaMSOptimize(StringRef PragmaName,
   return true;
 }
 
+/// Handle the Microsoft \#pragma intrinsic extension.
+///
+/// The syntax is:
+/// \code
+///  #pragma intrinsic(memset)
+///  #pragma intrinsic(strlen, memcpy)
+/// \endcode
+///
+/// Pragma intrisic tells the compiler to use a builtin version of the
+/// function. Clang does it anyway, so the pragma doesn't really do anything.
+/// Anyway, we emit a warning if the function specified in \#pragma intrinsic
+/// isn't an intrinsic in clang and suggest to include intrin.h, as well as
+/// declare the builtin if it has not been declared.
+bool Parser::HandlePragmaMSIntrinsic(StringRef PragmaName,
+                                     SourceLocation PragmaLocation) {
+  if (ExpectAndConsume(tok::l_paren, diag::warn_pragma_expected_lparen,
+                       PragmaName))
+    return false;
+
+  bool SuggestIntrinH = !PP.isMacroDefined("__INTRIN_H");
+
+  while (Tok.is(tok::identifier)) {
+    IdentifierInfo *II = Tok.getIdentifierInfo();
+    if (!II->getBuiltinID())
+      PP.Diag(Tok.getLocation(), diag::warn_pragma_intrinsic_builtin)
+          << II << SuggestIntrinH;
+    // If the builtin hasn't already been declared, declare it now.
+    DeclarationNameInfo NameInfo(II, Tok.getLocation());
+    LookupResult Previous(Actions, NameInfo, Sema::LookupOrdinaryName,
+                          RedeclarationKind::NotForRedeclaration);
+    Actions.LookupName(Previous, Actions.getCurScope(),
+                       /*CreateBuiltins*/ false);
+    if (Previous.empty())
+      Actions.LazilyCreateBuiltin(II, II->getBuiltinID(), Actions.getCurScope(),
+                                  /*ForRedeclaration*/ true, Tok.getLocation());
+    PP.Lex(Tok);
+    if (Tok.isNot(tok::comma))
+      break;
+    PP.Lex(Tok);
+  }
+  if (ExpectAndConsume(tok::r_paren, diag::warn_pragma_expected_rparen,
+                       PragmaName))
+    return false;
+
+  if (ExpectAndConsume(tok::eof, diag::warn_pragma_extra_tokens_at_eol,
+                       PragmaName))
+    return false;
+  return true;
+}
+
 void PragmaForceCUDAHostDeviceHandler::HandlePragma(
     Preprocessor &PP, PragmaIntroducer Introducer, Token &Tok) {
   Token FirstTok = Tok;
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..7eeb8d811fcfa
--- /dev/null
+++ b/clang/test/Sema/Inputs/builtin-system-header.h
@@ -0,0 +1,9 @@
+#ifdef USE_PRAGMA_BEFORE
+#pragma intrinsic(_InterlockedOr64)
+#endif
+
+#define MACRO(x,y) _InterlockedOr64(x,y);
+
+#ifdef USE_PRAGMA_AFTER
+#pragma intrinsic(_InterlockedOr64)
+#endif
diff --git a/clang/test/Sema/builtin-pragma-intrinsic-namespace.cpp b/clang/test/Sema/builtin-pragma-intrinsic-namespace.cpp
new file mode 100644
index 0000000000000..ffdc24b3d468a
--- /dev/null
+++ b/clang/test/Sema/builtin-pragma-intrinsic-namespace.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DOUTSIDE %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DINSIDE %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DNESTED %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DOUTSIDE -DEXTERN %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DINSIDE -DEXTERN %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility -fsyntax-only -verify -DNESTED -DEXTERN %s
+
+// expected-no-diagnostics
+#ifdef EXTERN
+extern "C"
+#endif
+unsigned __int64 _umul128(unsigned __int64, unsigned __int64,
+                          unsigned __int64 *);
+namespace {
+#ifdef INSIDE
+  #pragma intrinsic(_umul128)
+#endif
+#ifdef NESTED
+  namespace {
+#pragma intrinsic(_umul128)
+  }
+#endif
+}
+
+#ifdef OUTSIDE
+#pragma intrinsic(_umul128)
+#endif
+
+void foo() {
+  unsigned __int64 carry;
+  unsigned __int64 low = _umul128(0, 0, &carry);
+}
diff --git a/clang/test/Sema/builtin-pragma-intrinsic.c b/clang/test/Sema/builtin-pragma-intrinsic.c
new file mode 100644
index 0000000000000..1e8507bfd37df
--- /dev/null
+++ b/clang/test/Sema/builtin-pragma-intrinsic.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s -DUSE_PRAGMA_BEFORE
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s -DUSE_PRAGMA_AFTER
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s -DUSE_PRAGMA_AFTER_USE
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s -DUSE_PRAGMA_SAME_FILE
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -triple arm64-windows -isystem %S/Inputs %s
+
+#if defined(USE_PRAGMA_BEFORE) || defined(USE_PRAGMA_AFTER) || defined(USE_PRAGMA_SAME_FILE)
+// expected-no-diagnostics
+#else
+// expected-error@+10 {{call to undeclared library function '_InterlockedOr64'}}
+// expected-note@+9 {{include the header <intrin.h> or explicitly provide a declaration for '_InterlockedOr64'}}
+#endif
+#include <builtin-system-header.h>
+
+#ifdef USE_PRAGMA_SAME_FILE
+#pragma intrinsic(_InterlockedOr64)
+#endif
+
+void foo() {
+  MACRO(0,0);
+}
+
+#ifdef USE_PRAGMA_AFTER_USE
+#pragma intrinsic(_InterlockedOr64)
+#endif

[rnk]

Wow. Thanks for debugging it! I assumed this had more to do with intrin0.h than the namespaces in the libc++ string header.

[sarnex]

Sure, this was an interesting one. I thought I was losing my mind until I actually got into the debugger and saw the problem.