Don't create builtin declaration when looking for typo corrections in C++

[agutowski]

Summary

Declarations for the builtins were created when suspected of being corrections for a typo. That could trigger some absurd warnings about missing headers.

[thakis]

rtrieu: Is there some global "are we in typo correction?" bit somewhere already?

lib/CodeGen/CGBuiltin.cpp

800

Is this an unrelated change?

lib/Sema/SemaLookup.cpp

2013

Are you passing false anywhere? I can't find it (but I'm probably just blind) :-)

[agutowski]

lib/CodeGen/CGBuiltin.cpp

800

Yeah, mostly - I had to fix this for tests to work properly.

lib/Sema/SemaLookup.cpp

2013

Yes, it's not visible in the changelist - in line ~4305, in "LookupPotentialTypoResult":

SemaRef.LookupParsedName(Res, S, SS, /*AllowBuiltinCreation=*/false, EnteringContext);

[rsmith]

It sounds like this will hinder our ability to typo-correct to builtins. I think we only want to suppress implicitly declaring *library* builtins here (those that are expected to be provided by a header), not all builtins.

[rnk]

We shouldn't thread this kind of boolean all the way through name lookup. This is the kind of thing that callers should configure on the LookupResult object that they pass in.

[rnk]

Can you provide a more complete motivating example where our diagnostics were bad? I'm having a hard time figuring it out from the test

[agutowski]

In D25458#567697, @rnk wrote:

Can you provide a more complete motivating example where our diagnostics were bad? I'm having a hard time figuring it out from the test

Sure, compiling the following program in C++ for i386 on Windows emits warnings about implicit declarations of __emul and __emulu:

int main() {
  __umul(1, 2);
  return 0;
}

[agutowski]

It's part of a bigger problem (Clang shows warnings about functions guessed by typo-fixing in other cases as well), but it's not that important and I can't see any straighforward solution that would do more good than harm, so I'm closing this revision for now.

include/clang/Sema/Sema.h

       TypoRecoveryCallback;
 
 private:
-  bool CppLookupName(LookupResult &R, Scope *S);
+  bool CppLookupName(LookupResult &R, Scope *S, bool AllowBuiltinCreation);
 
   struct TypoExprState {
     std::unique_ptr<TypoCorrectionConsumer> Consumer;

lib/CodeGen/CGBuiltin.cpp

     Value *FnAssume = CGM.getIntrinsic(Intrinsic::assume);
     return RValue::get(Builder.CreateCall(FnAssume, ArgValue));
   }
+  case Builtin::BI_byteswap_ushort:
+  case Builtin::BI_byteswap_ulong:
+  case Builtin::BI_byteswap_uint64:
   case Builtin::BI__builtin_bswap16:
   case Builtin::BI__builtin_bswap32:
   case Builtin::BI__builtin_bswap64: {

lib/Sema/SemaLookup.cpp

 
 /// \brief Lookup a builtin function, when name lookup would otherwise
 /// fail.
-static bool LookupBuiltin(Sema &S, LookupResult &R) {
+static bool LookupBuiltin(Sema &S, LookupResult &R, bool AllowBuiltinCreation) {
   Sema::LookupNameKind NameKind = R.getLookupKind();
 
   // If we didn't find a use of this identifier, and if the identifier

 
       // If this is a builtin on this (or all) targets, create the decl.
       if (unsigned BuiltinID = II->getBuiltinID()) {
+        if (!AllowBuiltinCreation)
+          return false;
+
         // In C++ and OpenCL (spec v1.2 s6.9.f), we don't have any predefined
         // library functions like 'malloc'. Instead, we'll just error.
         if ((S.getLangOpts().CPlusPlus || S.getLangOpts().OpenCL) &&

 
 // Adds all qualifying matches for a name within a decl context to the
 // given lookup result.  Returns true if any matches were found.
-static bool LookupDirect(Sema &S, LookupResult &R, const DeclContext *DC) {
+static bool LookupDirect(Sema &S, LookupResult &R, const DeclContext *DC,
+                         bool AllowBuiltinCreation) {
   bool Found = false;
 
   // Lazily declare C++ special member functions.

     }
   }
 
-  if (!Found && DC->isTranslationUnit() && LookupBuiltin(S, R))
+  if (!Found && DC->isTranslationUnit() &&
+      LookupBuiltin(S, R, AllowBuiltinCreation))
     return true;
 
   if (R.getLookupName().getNameKind()

 }
 
 // Performs C++ unqualified lookup into the given file context.
-static bool
-CppNamespaceLookup(Sema &S, LookupResult &R, ASTContext &Context,
-                   DeclContext *NS, UnqualUsingDirectiveSet &UDirs) {
+static bool CppNamespaceLookup(Sema &S, LookupResult &R, ASTContext &Context,
+                               DeclContext *NS, UnqualUsingDirectiveSet &UDirs,
+                               bool AllowBuiltinCreation) {
 
   assert(NS && NS->isFileContext() && "CppNamespaceLookup() requires namespace!");
 
   // Perform direct name lookup into the LookupCtx.
-  bool Found = LookupDirect(S, R, NS);
+  bool Found = LookupDirect(S, R, NS, AllowBuiltinCreation);
 
   // Perform direct name lookup into the namespaces nominated by the
   // using directives whose common ancestor is this namespace.
   for (const UnqualUsingEntry &UUE : UDirs.getNamespacesFor(NS))
-    if (LookupDirect(S, R, UUE.getNominatedNamespace()))
+    if (LookupDirect(S, R, UUE.getNominatedNamespace(), AllowBuiltinCreation))
       Found = true;
 
   R.resolveKind();

 };
 } // end anonymous namespace
 
-bool Sema::CppLookupName(LookupResult &R, Scope *S) {
+bool Sema::CppLookupName(LookupResult &R, Scope *S, bool AllowBuiltinCreation) {
   assert(getLangOpts().CPlusPlus && "Can perform only C++ lookup");
 
   DeclarationName Name = R.getLookupName();

             VisitedUsingDirectives = true;
           }
 
-          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs)) {
+          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs,
+                                 AllowBuiltinCreation)) {
             R.resolveKind();
             return true;
           }

               "We should have been looking only at file context here already.");
 
           // Look into context considering using-directives.
-          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs))
+          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs,
+                                 AllowBuiltinCreation))
             Found = true;
         }
 

       }
   } else {
     // Perform C++ unqualified name lookup.
-    if (CppLookupName(R, S))
+    if (CppLookupName(R, S, AllowBuiltinCreation))
       return true;
   }
 
   // If we didn't find a use of this identifier, and if the identifier
   // corresponds to a compiler builtin, create the decl object for the builtin
   // now, injecting it into translation unit scope, and return it.
-  if (AllowBuiltinCreation && LookupBuiltin(*this, R))
+  if (LookupBuiltin(*this, R, AllowBuiltinCreation))
     return true;
 
   // If we didn't find a use of this identifier, the ExternalSource 

     // between LookupResults.
     bool UseLocal = !R.empty();
     LookupResult &DirectR = UseLocal ? LocalR : R;
-    bool FoundDirect = LookupDirect(S, DirectR, ND);
+    bool FoundDirect =
+        LookupDirect(S, DirectR, ND, /*AllowBuiltinCreation=*/true);
 
     if (FoundDirect) {
       // First do any local hiding.

     }
   } QL(LookupCtx);
 
-  if (LookupDirect(*this, R, LookupCtx)) {
+  if (LookupDirect(*this, R, LookupCtx, /*AllowBuiltinCreation=*/true)) {
     R.resolveKind();
     if (isa<CXXRecordDecl>(LookupCtx))
       R.setNamingClass(cast<CXXRecordDecl>(LookupCtx));

test/Sema/implicit-ms-builtin-decl.cpp

+// RUN: %clang_cc1 -triple x86_64-windows -fsyntax-only -verify %s -fms-extensions
+// RUN: %clang_cc1 -triple i386-windows -fsyntax-only -verify %s -fms-extensions
+
+void f() {
+  (void)_byteswap_ushort(42); // expected-error{{use of undeclared identifier '_byteswap_ushort'}}
+  (void)_byteswap_uint64(42LL); // expected-error{{use of undeclared identifier '_byteswap_uint64'}}
+}
+
+void _byteswap_ulong();
+
+unsigned short _byteswap_ushort(unsigned short);
+unsigned long long _byteswap_uint64(unsigned long long);
+
+void g() {
+  (void)_byteswap_ushort(42);
+  (void)_byteswap_uint64(42LL);
+}
+
+#if defined(__x86_64__)
+void h() {
+  (void)__mulh(21, 2); // expected-warning{{implicitly declaring library function '__mulh'}} \
+  // expected-note{{include the header <intrin.h> or explicitly provide a declaration for '__mulh'}}
+  (void)__umulh(21, 2); // expected-warning{{implicitly declaring library function '__umulh'}} \
+  // expected-note{{include the header <intrin.h> or explicitly provide a declaration for '__umulh'}}
+}
+
+long long __mulh(long long, long long);
+unsigned long long __umulh(unsigned long long, unsigned long long);
+
+void i() {
+  (void)__mulh(21, 2);
+  (void)__umulh(21, 2);
+}
+#endif
+
+#if defined(__i386__)
+void h() {
+  (void)__mulh(21LL, 2LL);  // expected-error{{use of undeclared identifier '__mulh'}}
+  (void)__umulh(21ULL, 2ULL);  // expected-error{{use of undeclared identifier '__umulh'}}
+}
+#endif