[PATCH] D77491: [Sema] Fix incompatible builtin redeclarations in non-global scope

[Raul Tambre via Phabricator via cfe-commits]

tambre created this revision.
tambre added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We didn't mark them previously as non-builtin in non-global scope if we determined they were incompatible redeclarations.
This caused problems down the line with them still being treated as builtins and having attributes added and the like.
Fix this by marking them non-builtin in any case. Handle redeclarations of such redeclarations as if they were redeclarations of the builtins by checking if the previous redeclaration was reverted from being a builtin.

Fixes PR45410.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77491

Files:
  clang/include/clang/Basic/IdentifierTable.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/builtin-redeclaration.c


Index: clang/test/CodeGen/builtin-redeclaration.c
===================================================================
--- /dev/null
+++ clang/test/CodeGen/builtin-redeclaration.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -emit-llvm-only %s
+
+// PR45410
+// Ensure we mark local extern redeclarations with a different type as non-builtin.
+void non_builtin() {
+  extern float exp();
+  exp(); // Will crash due to wrong number of arguments if this calls the builtin.
+}
+
+// PR45410
+// We mark exp() builtin as const with -fno-math-errno (default).
+// We mustn't do that for extern redeclarations of builtins where the type differs.
+float attribute() {
+  extern float exp();
+  return exp(1);
+}
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -3756,24 +3756,21 @@
   // If the previous declaration was an implicitly-generated builtin
   // declaration, then at the very least we should use a specialized note.
   unsigned BuiltinID;
-  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
+  bool RevertedBuiltin{Old->getIdentifier()->hasRevertedBuiltin()};
+  if (Old->isImplicit() &&
+      ((BuiltinID = Old->getBuiltinID()) || RevertedBuiltin)) {
     // If it's actually a library-defined builtin function like 'malloc'
     // or 'printf', just warn about the incompatible redeclaration.
-    if (Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID)) {
+    // Also warn if it's a redeclaration of a builtin redeclaration. We know if
+    // it is if the previous declaration is a reverted builtin.
+    if (RevertedBuiltin ||
+        Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID)) {
       Diag(New->getLocation(), diag::warn_redecl_library_builtin) << New;
       Diag(OldLocation, diag::note_previous_builtin_declaration)
         << Old << Old->getType();
 
-      // If this is a global redeclaration, just forget hereafter
-      // about the "builtin-ness" of the function.
-      //
-      // Doing this for local extern declarations is problematic.  If
-      // the builtin declaration remains visible, a second invalid
-      // local declaration will produce a hard error; if it doesn't
-      // remain visible, a single bogus local redeclaration (which is
-      // actually only a warning) could break all the downstream code.
-      if (!New->getLexicalDeclContext()->isFunctionOrMethod())
-        New->getIdentifier()->revertBuiltin();
+      // Forget hereafter about the "builtin-ness" of the function.
+      New->getIdentifier()->revertBuiltin();
 
       return false;
     }
Index: clang/include/clang/Basic/IdentifierTable.h
===================================================================
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -223,7 +223,7 @@
   }
   void setObjCKeywordID(tok::ObjCKeywordKind ID) { ObjCOrBuiltinID = ID; }
 
-  /// True if setNotBuiltin() was called.
+  /// True if revertBuiltin() was called.
   bool hasRevertedBuiltin() const {
     return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77491.255138.patch
Type: text/x-patch
Size: 3151 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200405/26ed5c2b/attachment.bin>