On May 22, 2014, at 1:58 PM, Richard Smith <[email protected]> wrote:

On Thu, May 22, 2014 at 1:48 PM, jahanian <[email protected]> wrote:

On May 22, 2014, at 11:40 AM, Richard Smith <[email protected]> wrote:


Another problem with our current model is that we build a broken redeclaration chain (the local declaration is marked as being a redeclaration of the implicitly-declared builtin, which it isn't). This leads to other bugs; for instance:

void f() { int memcpy(void); } void g() { memcpy(0,0,0); }

... misses the "implicitly declaring library function" warning.

Interesting point. Currently, we create a built-in declaration of “memcpy” and make local declaration as its redeclaration.
Should we just not create the implicit built-in declaration when user declaration is local? If we do this,
then the missing  warning you mentioned will come out (and my bug gets fixed). But, we will miss the warning 
about "incompatible redeclaration of library function ‘memcpy’” on local “redeclaration”.
Which, I think, is ok as it is no longer a redeclaration.
Am I reading you correctly?

I think we should still produce the "incompatible redeclaration of library function" warning. I think it'd be better if we didn't create the implicit built-in declaration at all, but I don't think it should matter whether the declaration is local. If I write a non-local declaration:

I have an experimental patch which does not create the  implicit built-in declaration at all. It fixes my original bug and also produces the error
for the test case that you sent earlier:

fix.c:1:16: warning: incompatible redeclaration of library function 'memcpy'
void f() { int memcpy(void); } 
               ^
fix.c:1:16: note: 'memcpy' is a builtin with type 'void *(void *, const void *, unsigned long)'
fix.c:3:12: warning: use of out-of-scope declaration of 'memcpy'
void g() { memcpy(0,0,0); }
           ^
fix.c:1:16: note: previous declaration is here
void f() { int memcpy(void); } 
               ^
fix.c:3:19: error: too many arguments to function call, expected 0, have 3
void g() { memcpy(0,0,0); }
           ~~~~~~ ^~~~~
fix.c:1:12: note: 'memcpy' declared here
void f() { int memcpy(void); } 
           ^
2 warnings and 1 error generated.

But there are several issues remaining.

1) This patch issues a diagnostic only once. So, for example, for this test:
void f() { int memcpy(void); } 
void g() { int memcpy(void); }

or:
void f() { int memcpy(void); } 
int memcpy(void) {}

We now issue the warning on first declaration of memcpy. Reason being that after 1st declaration we
treat memcpy as a user declaration and the 2nd declaration will not be seen as built-in
(you mentioned that there is a Builtin context I can check without requiring BuiltinID. But I
cannot find it).

Many tests had to be modified mainly because global declaration of builtin functions in these tests are
incorrect and this patch catches it. But this also points that projects will break.

Patch is attached for your further comments.
- Thanks, Fariborz
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp       (revision 209303)
+++ lib/Sema/SemaDecl.cpp       (working copy)
@@ -2734,38 +2734,15 @@
 
     // Fall through to diagnose conflicting types.
   }
-
-  // A function that has already been declared has been redeclared or
-  // defined with a different type; show an appropriate diagnostic.
-
   // 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())) {
-    // If it's actually a library-defined builtin function like 'malloc'
-    // or 'printf', just warn about the incompatible redeclaration.
-    if (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()->setBuiltinID(Builtin::NotBuiltin);
-
+  // This diagnostic is now being issued in one central location.
+  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID()))
       return false;
-    }
-
-    PrevDiag = diag::note_previous_builtin_declaration;
-  }
-
+    
+  // A function that has already been declared has been redeclared or
+  // defined with a different type; show an appropriate diagnostic.
   Diag(New->getLocation(), diag::err_conflicting_types) << New->getDeclName();
   Diag(OldLocation, PrevDiag) << Old << Old->getType();
   return true;
@@ -4322,10 +4299,11 @@
               R->isFunctionType())) {
       IsLinkageLookup = true;
       CreateBuiltins =
-          CurContext->getEnclosingNamespaceContext()->isTranslationUnit();
+          CurContext->getEnclosingNamespaceContext()->isTranslationUnit()
+            && !ForRedeclaration;
     } else if (CurContext->getRedeclContext()->isTranslationUnit() &&
                D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_static)
-      CreateBuiltins = true;
+      CreateBuiltins = !ForRedeclaration;
 
     if (IsLinkageLookup)
       Previous.clear(LookupRedeclarationWithLinkage);
@@ -4401,6 +4379,21 @@
     New = ActOnFunctionDeclarator(S, D, DC, TInfo, Previous,
                                   TemplateParamLists,
                                   AddToScope);
+    if (FunctionDecl *NewFD = dyn_cast<FunctionDecl>(New))
+      if (unsigned BuiltinID = NewFD->getBuiltinID()) {
+        ASTContext::GetBuiltinTypeError Error;
+        QualType T = Context.GetBuiltinType(BuiltinID, Error);
+          if (!T.isNull() && !Context.hasSameType(NewFD->getType(), T)) {
+          // If this is a redeclaration of a library builtin function,
+          // just forget hereafter about the "builtin-ness" of the function.
+          NewFD->getIdentifier()->setBuiltinID(Builtin::NotBuiltin);
+          if (Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID))
+            Diag(NewFD->getLocation(), diag::warn_redecl_library_builtin) << 
NewFD;
+          else
+            Diag(NewFD->getLocation(), diag::err_conflicting_types) << New;
+          Diag(NewFD->getLocation(), diag::note_previous_builtin_declaration) 
<< NewFD << T;
+        }
+      }
   } else {
     New = ActOnVariableDeclarator(S, D, DC, TInfo, Previous, 
TemplateParamLists,
                                   AddToScope);
Index: test/CodeGen/locally-redeclared-builtin-call.c
===================================================================
--- test/CodeGen/locally-redeclared-builtin-call.c      (revision 0)
+++ test/CodeGen/locally-redeclared-builtin-call.c      (working copy)
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+// rdar://16897451
+
+int Test()
+{
+  int a,b;
+  int pow (int,int);
+  return pow(a,b);
+}
+// CHECK-NOT: call i32 @llvm.pow.i32
+// CHECK: [[CALL:%.*]]  = call i32 @pow(i32 %0, i32 %1)
+// ret i32 [[CALL]]
+
+int pow(int c,int d)
+{
+  return c*d;
+}
Index: test/Sema/builtins-arm.c
===================================================================
--- test/Sema/builtins-arm.c    (revision 209534)
+++ test/Sema/builtins-arm.c    (working copy)
@@ -3,13 +3,15 @@
 // RUN:   -fsyntax-only -verify %s
 
 void f(void *a, void *b) {
-  __clear_cache(); // expected-error {{too few arguments to function call, 
expected 2, have 0}} // expected-note {{'__clear_cache' is a builtin with type 
'void (void *, void *)}}
+  __clear_cache(); // expected-error {{too few arguments to function call, 
expected 2, have 0}}
   __clear_cache(a); // expected-error {{too few arguments to function call, 
expected 2, have 1}}
   __clear_cache(a, b);
 }
 
-void __clear_cache(char*, char*); // expected-error {{conflicting types for 
'__clear_cache'}}
-void __clear_cache(void*, void*);
+void __clear_cache(char*, char*); // expected-error {{conflicting types for 
'__clear_cache'}} \
+                                 // expected-note {{'__clear_cache' is a 
builtin with type 'void (void *, void *)'}} \
+                                 // expected-note {{previous declaration is 
here}}
+void __clear_cache(void*, void*); // expected-error {{conflicting types for 
'__clear_cache'}}
 
 #if defined(__ARM_PCS) || defined(__ARM_EABI__)
 // va_list on ARM AAPCS is struct { void* __ap }.
Index: test/Sema/extern-redecl.c
===================================================================
--- test/Sema/extern-redecl.c   (revision 209558)
+++ test/Sema/extern-redecl.c   (working copy)
@@ -45,21 +45,23 @@
 
 // Test that invalid local extern declarations of library
 // builtins behave reasonably.
-extern void abort(void); // expected-note 2 {{previous declaration is here}}
+extern void abort(void); // expected-note 2 {{previous declaration is here}} \
+                         // expected-warning {{incompatible redeclaration of 
library function 'abort'}} \
+                         // expected-note {{'abort' is a builtin with type 
'void (void) __attribute__((noreturn))'}}
 extern float *calloc(); // expected-warning {{incompatible redeclaration of 
library function}} expected-note {{is a builtin}} expected-note 2 {{previous 
declaration is here}}
 void test5a() {
   int abort(); // expected-error {{conflicting types}}
-  float *malloc(); // expected-warning {{incompatible redeclaration of library 
function}} expected-note 2 {{is a builtin}}
+  float *malloc(); // expected-warning {{incompatible redeclaration of library 
function}} expected-note 1 {{is a builtin}}
   int *calloc(); // expected-error {{conflicting types}}
 }
 void test5b() {
   int abort(); // expected-error {{conflicting types}}
-  float *malloc(); // expected-warning {{incompatible redeclaration of library 
function}}
+  float *malloc();
   int *calloc(); // expected-error {{conflicting types}}
 }
 void test5c() {
   void (*_abort)(void) = &abort;
-  void *(*_malloc)() = &malloc;
+  void *(*_malloc)() = &malloc; // expected-error {{use of undeclared 
identifier 'malloc'}}
   float *(*_calloc)() = &calloc;
 }
 
Index: test/Sema/format-strings.c
===================================================================
--- test/Sema/format-strings.c  (revision 209303)
+++ test/Sema/format-strings.c  (working copy)
@@ -13,7 +13,8 @@
 int sprintf(char *restrict, const char *restrict, ...);
 int vasprintf(char **, const char *, va_list);
 int asprintf(char **, const char *, ...);
-int vfprintf(FILE *, const char *restrict, va_list);
+int vfprintf(FILE *, const char *restrict, va_list); // expected-warning 
{{incompatible redeclaration of library function 'vfprintf'}} \
+                                                    // expected-note 
{{'vfprintf' is a builtin with type 'int ()'}}
 int vprintf(const char *restrict, va_list);
 int vsnprintf(char *, size_t, const char *, va_list);
 int vsprintf(char *restrict, const char *restrict, va_list); // 
expected-note{{passing argument to parameter here}}
Index: test/Sema/implicit-builtin-decl.c
===================================================================
--- test/Sema/implicit-builtin-decl.c   (revision 209303)
+++ test/Sema/implicit-builtin-decl.c   (working copy)
@@ -35,7 +35,7 @@
 int a[10];
 
 int f0() {
-  return __builtin_object_size(&a); // expected-error {{too few arguments to 
function}}
+  return __builtin_object_size(&a); // expected-error {{returning 'void' from 
a function with incompatible result type 'int'}}
 }
 
 void * realloc(void *p, int size) { // expected-warning{{incompatible 
redeclaration of library function 'realloc'}} \
Index: test/Sema/knr-def-call.c
===================================================================
--- test/Sema/knr-def-call.c    (revision 209303)
+++ test/Sema/knr-def-call.c    (working copy)
@@ -19,11 +19,12 @@
 
 // <rdar://problem/8193107>
 void f4() {
-    char *rindex();
+    char *rindex(); // expected-warning {{incompatible redeclaration of 
library function 'rindex'}} \
+                   // expected-note {{'rindex' is a builtin with type 'char 
*(const char *, int)'}}
 }
 
 char *rindex(s, c)
-     register char *s, c; // expected-warning{{promoted type 'char *' of K&R 
function parameter is not compatible with the parameter type 'const char *' 
declared in a previous prototype}}
+     register char *s, c;
 {
   return 0;
 }
Index: test/Sema/knr-variadic-def.c
===================================================================
--- test/Sema/knr-variadic-def.c        (revision 209303)
+++ test/Sema/knr-variadic-def.c        (working copy)
@@ -20,7 +20,7 @@
         return x;
 }
 
-void exit();
+void exit(int) __attribute__((noreturn));
 
 int main(argc,argv)
         int argc;char**argv;
Index: test/Sema/return.c
===================================================================
--- test/Sema/return.c  (revision 209303)
+++ test/Sema/return.c  (working copy)
@@ -192,7 +192,7 @@
 void test28() __attribute__((noreturn));
 void test28(x) { while (1) { } }
 
-void exit(int);
+void exit(int) __attribute__((noreturn));
 int test29() {
   exit(1);
 }
Index: test/Sema/vfprintf-valid-redecl.c
===================================================================
--- test/Sema/vfprintf-valid-redecl.c   (revision 209303)
+++ test/Sema/vfprintf-valid-redecl.c   (working copy)
@@ -7,7 +7,7 @@
 // Clang has defined 'vfprint' in builtin list. If the following line occurs 
before any other
 // `vfprintf' in this file, and we getPreviousDecl()->getTypeSourceInfo() on 
it, then we will
 // get a null pointer since the one in builtin list doesn't has valid 
TypeSourceInfo.
-int vfprintf(void) { return 0; }
+int vfprintf() { return 0; }
 #endif
 
 // PR4290






  int memcpy(void);

... I should not get a redecl chain for memcpy with two declarations with different parameter types.

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to