[PATCH] Enable checkUndefinedButUsed() unconditionally at end of TU

[Alp Toker]

Right now we only emit the "function/variable has internal linkage but 
is not defined" warning at end of TU if no errors were encountered.

However we probably shouldn't skip checkUndefinedButUsed() so liberally. 
It causes lack of test coverage (doesn't appear in any -verify + 
expected-error test) and loss of useful information for users.

Best guess is that it came to be this way back when bodies used to get 
skipped liberally in error recovery. Nowadays we're tracking definitions 
effectively and should be able to enable the checks unconditionally.

Simple enough? Now for the bad news..

There are 26 tests relying on not using undefined inline functions:

Failing Tests (26):
     Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
     Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p8.cpp
     Clang :: CXX/drs/dr2xx.cpp
     Clang :: CXX/drs/dr3xx.cpp
     Clang :: CXX/drs/dr4xx.cpp
     Clang :: CXX/expr/expr.const/p2-0x.cpp
     Clang :: CXX/expr/expr.prim/expr.prim.lambda/p19.cpp
     Clang :: CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
     Clang :: CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
     Clang :: CXX/temp/temp.arg/temp.arg.type/p2.cpp
     Clang :: CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p11.cpp
     Clang :: Modules/namespaces.cpp
     Clang :: Modules/redecl-add-after-load.cpp
     Clang :: Parser/cxx-ambig-paren-expr.cpp
     Clang :: Parser/recovery.cpp
     Clang :: Sema/array-init.c
     Clang :: Sema/inline.c
     Clang :: Sema/invalid-decl.c
     Clang :: SemaCXX/anonymous-struct.cpp
     Clang :: SemaCXX/explicit.cpp
     Clang :: SemaCXX/expression-traits.cpp
     Clang :: SemaCXX/lambda-expressions.cpp
     Clang :: SemaCXX/new-delete.cpp
     Clang :: SemaCXX/overload-call.cpp
     Clang :: SemaCXX/typo-correction.cpp
     Clang :: SemaTemplate/deduction.cpp

For each we need to figure out the right fix, which varies case by case:

  a) add a definition body {}
  b) de-inline the declaration
  c) add expected-warning {{}}
  d) or just run -Wno-undefined-internal in tests where it doesn't matter

* (c) and (d) will be problematic when the warning becomes a hard error 
as planned in Sema FIXMEs.

So, if we want to apply the patch fixing the issue we'll need to update 
all those tests to valid syntax.

The attached patch implements the fix and demonstrates each one of the 
strategies to get started. Thoughts/volunteers? :-)

Related to:
   PR19910 - Don't suppress unused/undefined warnings when there are errors
   http://llvm.org/bugs/show_bug.cgi?id=19910

Historical context:
   PR5933 - bogus unused variable warning after error
   http://llvm.org/bugs/show_bug.cgi?id=5933

Alp.

-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp
index ce0911e..b2e084b 100644
--- a/lib/Sema/Sema.cpp
+++ b/lib/Sema/Sema.cpp
@@ -468,6 +468,8 @@ void Sema::getUndefinedButUsed(
 /// checkUndefinedButUsed - Check for undefined objects with internal linkage
 /// or that are inline.
 static void checkUndefinedButUsed(Sema &S) {
+  if (auto ES = S.getExternalSource())
+    ES->ReadUndefinedButUsed(S.UndefinedButUsed);
   if (S.UndefinedButUsed.empty()) return;
 
   // Collect all the still-undefined entities with internal linkage.
@@ -817,12 +819,10 @@ void Sema::ActOnEndOfTranslationUnit() {
         }
       }
     }
-
-    if (ExternalSource)
-      ExternalSource->ReadUndefinedButUsed(UndefinedButUsed);
-    checkUndefinedButUsed(*this);
   }
 
+  checkUndefinedButUsed(*this);
+
   if (Diags.getDiagnosticLevel(diag::warn_unused_private_field,
                                SourceLocation())
         != DiagnosticsEngine::Ignored) {
diff --git a/test/CXX/drs/dr4xx.cpp b/test/CXX/drs/dr4xx.cpp
index 815dbfc..39a68dc 100644
--- a/test/CXX/drs/dr4xx.cpp
+++ b/test/CXX/drs/dr4xx.cpp
@@ -1078,7 +1078,7 @@ namespace dr487 { // dr487: yes
 }
 
 namespace dr488 { // dr488: yes c++11
-  template <typename T> void f(T);
+  template <typename T> void f(T); // expected-warning {{has internal linkage but is not defined}}
   void f(int);
   void g() {
     // FIXME: It seems CWG thought this should be a SFINAE failure prior to
@@ -1086,7 +1086,7 @@ namespace dr488 { // dr488: yes c++11
     // allow local types as template arguments or treat this as a SFINAE
     // failure.
     enum E { e };
-    f(e);
+    f(e); // expected-note {{used here}}
 #if __cplusplus < 201103L
     // expected-error at -2 {{local type}}
 #endif
diff --git a/test/Sema/inline.c b/test/Sema/inline.c
index 8a3835b..b7675b2 100644
--- a/test/Sema/inline.c
+++ b/test/Sema/inline.c
@@ -7,7 +7,7 @@
 
 // Check the use of static variables in non-static inline functions.
 static int staticVar; // expected-note + {{'staticVar' declared here}}
-static int staticFunction(); // expected-note + {{'staticFunction' declared here}}
+static int staticFunction() { return 0; }; // expected-note + {{'staticFunction' declared here}}
 static struct { int x; } staticStruct; // expected-note + {{'staticStruct' declared here}}
 
 inline int useStatic () { // expected-note 3 {{use 'static' to give inline function 'useStatic' internal linkage}}
@@ -40,6 +40,8 @@ inline int useConst () {
   return constFunction(); // no-warning
 }
 
+static int constFunction() { return 0; }
+
 #else
 // -------
 // This is the main source file.
diff --git a/test/Sema/invalid-decl.c b/test/Sema/invalid-decl.c
index f0954b0..c06aae1 100644
--- a/test/Sema/invalid-decl.c
+++ b/test/Sema/invalid-decl.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-undefined-internal -verify
 
 void test() {
     char = 4;  // expected-error {{expected identifier}}
diff --git a/test/SemaCXX/overload-call.cpp b/test/SemaCXX/overload-call.cpp
index da28eef..f040c1c 100644
--- a/test/SemaCXX/overload-call.cpp
+++ b/test/SemaCXX/overload-call.cpp
@@ -346,7 +346,7 @@ namespace test2 {
   class qrgb666 {
     inline operator unsigned int () const;
 
-    inline bool operator==(const qrgb666 &v) const;
+    bool operator==(const qrgb666 &v) const;
     inline bool operator!=(const qrgb666 &v) const { return !(*this == v); }
   };
 }