PR20958 Allow redeclaration of type-generic builtins

[• chatur01]

Summary

This patch allows the user to declare the C99 math builtins that take the generic "real-floating" argument types.
A use case is described in https://llvm.org/bugs/show_bug.cgi?id=20958 -- this is preventing Clang from being used to build Android.

In C++98 and C89, you may redeclare these builtins to take any type, Clang will pretend these don't exist in these languages.
In C++11 and C99 (and derivations thereof), you can only redeclare the builtin with types that are "real-floating". If you try to redeclare with different types, a diagnostic is produced.

I also tweaked the __noop builtin to specify that it's arguments are custom type-checked. This was avoid a semantic error about passing non-POD types through a variadic argument, which is not appropriate for that builtin.

[• chatur01]

Ping. Should make it clearer that this must be committed after D9913.

[• chatur01]

Handle signbit as well.

[• chatur01]

Ping :)

[• chatur01]

Ping.

[• chatur01]

Weekly ping.

[ilya-palachev]

Hi! Thanks for this patch.

We're building Android6 with Clang (in order to apply static analyzer on it), and without this patch we've 613 build failures. This patch helps to get rid of 521 of those build failures (85%).

"Works for me".

lib/Sema/SemaDecl.cpp

1758

Trailing space

1763

Ditto

1775

Ditto

1837

Ditto

include/clang/Basic/Builtins.def

 LANGBUILTIN(__GetExceptionInfo, "v*.", "ntu", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedCompareExchange, "LiLiD*LiLi", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedCompareExchangePointer, "v*v*D*v*v*", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedDecrement,       "LiLiD*",     "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchangeAdd,     "LiLiD*Li",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchange,        "LiLiD*Li",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchangePointer, "v*v*D*v*",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedIncrement,       "LiLiD*",     "n", ALL_MS_LANGUAGES)
-LANGBUILTIN(__noop,           "i.",  "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__noop,           "i.",  "nt", ALL_MS_LANGUAGES)
 LANGBUILTIN(__readfsdword,    "ULiULi", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(__va_start,       "vc**.", "nt", ALL_MS_LANGUAGES)
 
 // Microsoft library builtins.
 LIBBUILTIN(_setjmpex, "iJ", "fj",   "setjmpex.h", ALL_MS_LANGUAGES)
 
 // C99 library functions
 // C99 stdlib.h

include/clang/Basic/DiagnosticSemaKinds.td

def warn_strncat_large_size : Warning<
   "buffer overflow">, InGroup<StrncatSize>;
 def warn_strncat_src_size : Warning<"size argument in 'strncat' call appears " 
   "to be size of the source">, InGroup<StrncatSize>;
 def warn_strncat_wrong_size : Warning<
   "the value of the size argument to 'strncat' is wrong">, InGroup<StrncatSize>;
 def note_strncat_wrong_size : Note<
   "change the argument to be the free space in the destination buffer minus " 
   "the terminating null byte">;
-
+def err_incorrect_args_builtin_redecl : Error<
+  "%0 is a builtin that takes %1 argument%s1, it has been redeclared with %2 "
+  "argument%s2">;
+def err_builtin_expects_realfloating_type : Error<
+  "%0 expects either double, long double or float argument types, but has been "
+  "given an argument of type %1">;
 def warn_assume_side_effects : Warning<
   "the argument to %0 has side effects that will be discarded">,
   InGroup<DiagGroup<"assume">>;
 
 def warn_memcpy_chk_overflow : Warning<
   "%0 will always overflow destination buffer">,
   InGroup<DiagGroup<"builtin-memcpy-chk-size">>;
 

include/clang/Sema/Sema.h

private:
   /// \brief The parser's current scope.
   ///
   /// The parser maintains this state here.
   Scope *CurScope;
 
   mutable IdentifierInfo *Ident_super;
   mutable IdentifierInfo *Ident___float128;
 
+private:
+  using BuiltinTypeBinding = std::pair<unsigned, const Type *>;
+  /// \brief All the declaration types of builtin libc/libm functions seen.
+  SmallVector<BuiltinTypeBinding, 2> SavedBuiltinFunctionDeclarations;
+
+  /// \brief Create an appropriate declaration for a type-generic builtin.
+  /// If a user-specified declaration of a type-generic builtin has
+  /// been seen in the TU (tracked by \c SavedBuiltinFunctionDeclarations), use
+  /// that if it contains legal arguments. If the argument types are illegal
+  /// emit a variadic declaration in C++, and a no-arg declaration in C as
+  /// fall-back cases.
+  ///
+  /// If the language is not C++11 or C99 (or derivations thereof), return
+  /// whatever \c GetBuiltinType would return.
+  ///
+  /// \param [in] II  The \c IdentifierInfo for the declaration name.
+  /// \param [in] ID  The identifier ID for the detected compiler builtin.
+  /// \param [in] Loc The location this declaration was found at.
+  /// \param [in] BT  The type computed from GetBuiltinType, used a fall-back
+  /// case if we can't be more specific.
+  ///
+  /// \return A declaration type for this builtin, or \c BT if no improvement
+  /// can be made.
+  QualType CheckGenericBuiltinRedeclaration(IdentifierInfo *II, unsigned ID,
+                                            SourceLocation Loc, QualType BT);
+
 protected:
   friend class Parser;
   friend class InitializationSequence;
   friend class ASTReader;
   friend class ASTDeclReader;
   friend class ASTWriter;
 
 public:

lib/AST/ASTContext.cpp

QualType ASTContext::GetBuiltinType(unsigned Id,
   assert((TypeStr[0] != '.' || TypeStr[1] == 0) &&
          "'.' should only occur at end of builtin type list!");
 
   FunctionType::ExtInfo EI(CC_C);
   if (BuiltinInfo.isNoReturn(Id)) EI = EI.withNoReturn(true);
 
   bool Variadic = (TypeStr[0] == '.');
 
-  // We really shouldn't be making a no-proto type here, especially in C++.
-  if (ArgTypes.empty() && Variadic)
+  // We really shouldn't be making a no-proto type here.
+  if (ArgTypes.empty() && Variadic && !LangOpts.CPlusPlus)
     return getFunctionNoProtoType(ResType, EI);
 
   FunctionProtoType::ExtProtoInfo EPI;
   EPI.ExtInfo = EI;
   EPI.Variadic = Variadic;
 
   return getFunctionType(ResType, ArgTypes, EPI);
 }

lib/Sema/SemaDecl.cpp

static StringRef getHeaderName(ASTContext::GetBuiltinTypeError Error) {
   case ASTContext::GE_Missing_setjmp:
     return "setjmp.h";
   case ASTContext::GE_Missing_ucontext:
     return "ucontext.h";
   }
   llvm_unreachable("unhandled error kind");
 }
 
+
+QualType Sema::CheckGenericBuiltinRedeclaration(IdentifierInfo *II, unsigned ID,
+                                                SourceLocation Loc,
+                                                QualType BuiltinType) {
+  bool HasC99MathLib = LangOpts.CPlusPlus11 || LangOpts.C99;
+
+  FunctionType::ExtInfo EI(CC_C);
+  FunctionProtoType::ExtProtoInfo EPI;
+  EPI.ExtInfo = EI;
+  EPI.Variadic = LangOpts.CPlusPlus11;
+  SmallVector<QualType, 2> ArgTypes;
+
+  auto DefaultFunctionType = LangOpts.CPlusPlus11 ?
+    Context.getFunctionType(Context.IntTy, ArrayRef<QualType>(), EPI) :
+    Context.getFunctionNoProtoType(Context.IntTy, EI);
+
+  // Check SavedBuiltinFunctionDeclarations for an instance of the builtin
+  // ID.
+  auto SeenDecl = std::find_if(SavedBuiltinFunctionDeclarations.begin(),
+                               SavedBuiltinFunctionDeclarations.end(),
+                               [ID] (const BuiltinTypeBinding &B) {
+                                 return B.first == ID;
+                               });
+
+  if (SeenDecl != SavedBuiltinFunctionDeclarations.end()) {
+    const Type *TP = SeenDecl->second;
+
+    if (TP->isFunctionProtoType()) {
+      if (const FunctionProtoType *FPT = dyn_cast<FunctionProtoType>(TP)) {
+        // Ignore if we're looking at a variadic prototype like T name(...)
+        // In C99, the semantic checks for function merging will complain
+        // about having no parameter before the ellipses.
+        if (FPT->getNumParams()) {
+          switch (ID) {
+          case Builtin::BI__builtin_isgreater:
+          case Builtin::BI__builtin_isgreaterequal:
+          case Builtin::BI__builtin_isless:
+          case Builtin::BI__builtin_islessequal:
+          case Builtin::BI__builtin_islessgreater:
+          case Builtin::BI__builtin_isunordered:
+            // These builtins take two real-floating arguments, all that is
+            // done here is to check that if they've been declared with
+            // two real-floating arguments, use that as the declaration of
+            // the builtin.
+            {
+              if (FPT->getNumParams() != 2 && HasC99MathLib) {
+                Diag(Loc, diag::err_incorrect_args_builtin_redecl)
+                  << II->getName()
+                  << 2
+                  << FPT->getNumParams();
+                return DefaultFunctionType;
+              }
+              
+              QualType QT1 = FPT->getParamType(0);
+              QualType QT2 = FPT->getParamType(1);
+              ArgTypes.push_back(QT1);
+              ArgTypes.push_back(QT2);
+              
+              if (QT1->isRealFloatingType() && QT2->isRealFloatingType()) {
+                EPI.Variadic = false;
+                return Context.getFunctionType(Context.IntTy, ArgTypes, EPI);
+              } else {
+                if (HasC99MathLib) {
+                  // Emit a diagnostic here since the user has attempted to
+                  // redeclare a standard builtin with erroneous argument types.
+                  if (QT1->isRealFloatingType())
+                    QT1 = QT2;
+                  Diag(Loc, diag::err_builtin_expects_realfloating_type)
+                    << II->getName()
+                    << QT1.getAsString();              
+                  return DefaultFunctionType;
+                } else {
+                  // Return whatever the user defined. C99 builtins don't exist.
+                  EPI.Variadic = false;
+                  return Context.getFunctionType(Context.IntTy, ArgTypes, EPI);
+                }
+              }
+              break;
+            }
+          case Builtin::BI__builtin_isfinite:
+          case Builtin::BI__builtin_isinf:
+          case Builtin::BI__builtin_isinf_sign:
+          case Builtin::BI__builtin_isnormal:
+          case Builtin::BI__builtin_isnan:
+          case Builtin::BI__builtin_signbit:
+            {
+              QualType QT = FPT->getParamType(0);
+              ArgTypes.push_back(QT);
+
+              if (QT->isRealFloatingType()) {
+                EPI.Variadic = false;
+                return Context.getFunctionType(Context.IntTy, ArgTypes, EPI);
+              } else {
+                if (HasC99MathLib) {
+                  // Emit a diagnostic here since the user has attempted to
+                  // redeclare a standard builtin with an erroneous argument type.
+                  Diag(Loc, diag::err_builtin_expects_realfloating_type)
+                    << II->getName()
+                    << QT.getAsString();
+                  return DefaultFunctionType;
+                } else {
+                  // Return whatever the user defined. C99 builtins don't exist.
+                  EPI.Variadic = false;
+                  return Context.getFunctionType(Context.IntTy, ArgTypes, EPI);
+                }
+              }
+              break;
+            }
+          }
+        }
+      }
+    } else if (TP->isFunctionNoProtoType() && HasC99MathLib) {
+      // This is a K&R style int __builtin_name() declaration.
+      switch (ID) {
+      case Builtin::BI__builtin_isgreater:
+      case Builtin::BI__builtin_isgreaterequal:
+      case Builtin::BI__builtin_isless:
+      case Builtin::BI__builtin_islessequal:
+      case Builtin::BI__builtin_islessgreater:
+      case Builtin::BI__builtin_isunordered:
+        if (HasC99MathLib)
+          Diag(Loc, diag::err_incorrect_args_builtin_redecl)
+            << II->getName()
+            << 2
+            << 0;
+        break;
+      case Builtin::BI__builtin_isfinite:
+      case Builtin::BI__builtin_isinf:
+      case Builtin::BI__builtin_isinf_sign:
+      case Builtin::BI__builtin_isnormal:
+      case Builtin::BI__builtin_isnan:
+      case Builtin::BI__builtin_signbit:        
+        Diag(Loc, diag::err_incorrect_args_builtin_redecl)
+          << II->getName()
+          << 1
+          << 0;
+        break;
+      }
+      return DefaultFunctionType;
+    }
+  }
+
+  // Default to just returning whatever GetBuiltinType found.
+  return BuiltinType;
+}
+
 /// LazilyCreateBuiltin - The specified Builtin-ID was first used at
 /// file scope.  lazily create a decl for it. ForRedeclaration is true
 /// if we're creating this built-in in anticipation of redeclaring the
 /// built-in.
 NamedDecl *Sema::LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID,
                                      Scope *S, bool ForRedeclaration,
                                      SourceLocation Loc) {
   LookupPredefedObjCSuperType(*this, S, II);
 
   ASTContext::GetBuiltinTypeError Error;
   QualType R = Context.GetBuiltinType(ID, Error);
+
+  R = CheckGenericBuiltinRedeclaration(II, ID, Loc, R);
+
   if (Error) {
     if (ForRedeclaration)
       Diag(Loc, diag::warn_implicit_decl_requires_sysheader)
           << getHeaderName(Error)
           << Context.BuiltinInfo.GetName(ID);
     return nullptr;
   }
 
else if (CurContext->isFunctionOrMethod() &&
           CurContext->getEnclosingNamespaceContext()->isTranslationUnit();
     } else if (CurContext->getRedeclContext()->isTranslationUnit() &&
                D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_static)
       CreateBuiltins = true;
 
     if (IsLinkageLookup)
       Previous.clear(LookupRedeclarationWithLinkage);
 
+    if (IdentifierInfo *II = Name.getAsIdentifierInfo())
+      if (unsigned BuiltinID = II->getBuiltinID())
+        if (Context.BuiltinInfo.isLibFunction(BuiltinID))
+          // Keep the declaration of this library function for later processing
+          // if/when we're asking to generate a declaration for it. (See
+          // LazilyCreateBuiltin). For type-generic builtins, keeping the user
+          // declaration around can inform what declaration we should
+          // actually create.
+          SavedBuiltinFunctionDeclarations.push_back(std::make_pair(BuiltinID,
+                                                                    R.getTypePtr()));
     LookupName(Previous, S, CreateBuiltins);
   } else { // Something like "int foo::x;"
     LookupQualifiedName(Previous, DC);
 
     // C++ [dcl.meaning]p1:
     //   When the declarator-id is qualified, the declaration shall refer to a 
     //  previously declared member of the class or namespace to which the 
     //  qualifier refers (or, in the case of a namespace, of an element of the

test/Sema/builtins-c89-valid.c

+// RUN: %clang_cc1 -std=c89 %s -fsyntax-only -verify -triple=i686-mingw32
+// expected-no-diagnostics
+// In C89, pretend these builtins don't exist.
+
+int __builtin_isnan(double);
+int __builtin_isinf(float);
+int __builtin_isfinite(int);
+int __builtin_isunordered(double, float);
+int __builtin_islessgreater();

test/Sema/builtins-c99-generic.c

+// RUN: %clang_cc1 -std=c99 %s -fsyntax-only -verify -triple=i686-mingw32
+
+int __builtin_isnan(double);
+int __builtin_isinf(float);
+
+int __builtin_isunordered(double, float);
+int __builtin_islessgreater(long double, double);
+int __builtin_isgreater(float, float);
+
+int __builtin_isfinite(int); // expected-error {{__builtin_isfinite expects either double, long double or float argument types, but has been given an argument of type int}}
+int __builtin_isnormal(); // expected-error {{__builtin_isnormal is a builtin that takes 1 argument, it has been redeclared with 0 arguments}}
+int __builtin_islessequal(double); // expected-error {{__builtin_islessequal is a builtin that takes 2 arguments, it has been redeclared with 1 argument}}
+int __builtin_isless(); // expected-error {{__builtin_isless is a builtin that takes 2 arguments, it has been redeclared with 0 arguments}}

test/SemaCXX/builtins.cpp

namespace addressof {
   S *ptmp = __builtin_addressof(S{}); // expected-error {{taking the address of a temporary}}
 }
 
 void no_ms_builtins() {
   __assume(1); // expected-error {{use of undeclared}}
   __noop(1); // expected-error {{use of undeclared}}
   __debugbreak(); // expected-error {{use of undeclared}}
 }
+
+// Make sure the processing for type-generic bultins allows the usual C++ overloading.
+int __builtin_isnan(double);
+int __builtin_isnan(long double);
+int __builtin_isnan(float);
+int __builtin_isnan(...);