[Sema] PR23090 Crash when return type or parameter types for extern "C" functions don't have external linkage

[hintonda]

Summary

Added checks to make sure the return type and parameter types for functions
declared as extern "C" have external linkage.

[hintonda]

Here's an example of when this will cause a crash:

namespace {
Struct G;
extern "C" G *f(); has external linkage
G *f(); will assert since it has internal linkage because of G and doesn't match previous declaration
}

[hintonda]

This also causes a few other test failures (~8) which will need to be fixed if this patch is accepted.

[hintonda]

This change breaks a couple tests in test/SemaCXX/linkage.cpp where we no longer allow mixing internal and external linkage, i.e., extern "C" functions have external linkage no matter where they are declared, but return types and parameters declared in anonymous namespaces have internal linkage.

Not yet sure how to go about fixing those, but one was based on PR9316, however, the test is more complicated and is invalid -- so we don't generate code.

[hintonda]

Fixed additional tests.

[hintonda]

• Fix remaining test failures caused by linkage errors.

[rsmith]

Why do you think this is ill-formed? Using a type with internal linkage as the type of an extern "C" function is explicitly permitted by the C++ standard. [basic.link]p8 says:

"A type without linkage shall not be used as the type of a variable or function with external linkage unless the entity has C language linkage [...]."

include/clang/Basic/DiagnosticSemaKinds.td

def warn_unneeded_static_internal_decl : Warning<
   "should be declared 'static inline'">,
   InGroup<UnneededInternalDecl>, DefaultIgnore;
 def warn_unneeded_member_function : Warning<
   "member function %0 is not needed and will not be emitted">,
   InGroup<UnneededMemberFunction>, DefaultIgnore;
 def warn_unused_private_field: Warning<"private field %0 is not used">,
   InGroup<UnusedPrivateField>, DefaultIgnore;
 
+def err_return_value_linkage: Error<
+  "%0 has C-linkage specified, but return type %1 has internal linkage">;
+def err_parameter_value_linkage: Error<
+  "%0 has C-linkage specified, but parameter type %1 has internal linkage">;
+
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_size: Warning<
   "return value of %0 is a large (%1 bytes) pass-by-value object; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_udt: Warning<
   "%0 has C-linkage specified, but returns user-defined type %1 which is "

lib/Sema/SemaDecl.cpp

if (NewFD->isInvalidDecl()) {
       // Note that this is not the case for explicit specializations of
       // function templates or member functions of class templates, per
       // C++ [temp.expl.spec]p2. We also allow these declarations as an 
       // extension for compatibility with old SWIG code which likes to 
       // generate them.
       Diag(NewFD->getLocation(), diag::ext_out_of_line_declaration)
         << D.getCXXScopeSpec().getRange();
     }
+
+    if (NewFD->isExternC()) {
+      // Check return type linkage
+      QualType R = NewFD->getReturnType();
+      if (R.getTypePtr()->getLinkage() != Linkage::ExternalLinkage) {
+        Diag(NewFD->getLocation(), diag::err_return_value_linkage)
+          << NewFD << R;
+        NewFD->setInvalidDecl();
+      }
+      // Check parameter type linkage
+      for (auto param : NewFD->parameters()) {
+        QualType P = param->getOriginalType();
+        if (P.getTypePtr()->getLinkage() != Linkage::ExternalLinkage) {
+          Diag(NewFD->getLocation(), diag::err_parameter_value_linkage)
+            << NewFD << P;
+          NewFD->setInvalidDecl();
+        }
+      }
+    }
   }
 
   ProcessPragmaWeak(S, NewFD);
   checkAttributesAfterMerging(*this, *NewFD);
 
   AddKnownFunctionAttributes(NewFD);
 
   if (NewFD->hasAttr<OverloadableAttr>() && 

test/CXX/drs/dr3xx.cpp

 
 namespace dr319 { // dr319: no
   // FIXME: dup dr389
   // FIXME: We don't have a diagnostic for a name with linkage
   //        having a type without linkage.
   typedef struct {
     int i;
   } *ps;
-  extern "C" void f(ps);
-  void g(ps); // FIXME: ill-formed, type 'ps' has no linkage
+  extern "C" void f(ps); // expected-error-re {{'f' has C-linkage specified, but parameter type 'ps' (aka 'dr319::(anonymous struct {{.*}} *') has internal linkage}}
+  void g(ps);
 
   static enum { e } a1;
   enum { e2 } a2; // FIXME: ill-formed, enum type has no linkage
 
   enum { n1 = 1u };
   typedef int (*pa)[n1];
   pa parr; // ok, type has linkage despite using 'n1'
 

test/Sema/pr23090-crash-on-invalid.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash (PR23090).
+
+namespace {
+
+// check return type
+struct A;
+extern "C" A *foo(); // expected-error {{'foo' has C-linkage specified, but return type '(anonymous namespace)::A *' has internal linkage}}
+A *foo();
+
+// check parameter
+struct B;
+extern "C" void bar(B*); // expected-error {{'bar' has C-linkage specified, but parameter type '(anonymous namespace)::B *' has internal linkage}}
+void bar(B*);
+
+}

test/SemaCXX/linkage.cpp

namespace test2 {
 
   void test(A *a) {
     a->foo(0);
   }
 }
 
 namespace test3 {
   namespace { struct A {}; }
+  struct B {};
 
   // CHECK: define internal void @_ZN5test34testENS_12_GLOBAL__N_11AE(
   void test(A a) {}
   void force() { test(A()); }
 
   // CHECK: define void @test3(
-  extern "C" void test3(A a) {}
+  extern "C" void test3(B b) {}
 }
 
 namespace {
   // CHECK: define void @test4(
   extern "C" void test4(void) {}
 }
 
-// PR9316: Ensure that even non-namespace-scope function declarations in
-// a C declaration context respect that over the anonymous namespace.
-extern "C" {
-  namespace {
-    struct X {
-      int f() {
-        extern int g();
-        extern int a;
-
-        // Test both for mangling in the code generation and warnings from use
-        // of internal, undefined names via -Werror.
-        // CHECK: call i32 @g(
-        // CHECK: load i32, i32* @a,
-        return g() + a;
-      }
-    };
-  }
-  // Force the above function to be emitted by codegen.
-  int test(X& x) {
-    return x.f();
-  }
-}
-
 // CHECK: define linkonce_odr i8* @_ZN5test11A3fooILj0EEEPvv(
 // CHECK: define linkonce_odr i8* @_ZN5test21A1BILj0EE3fooEv(
 
 namespace test5 {
   struct foo {
   };
   extern "C" {
     const foo bar[]  = {

test/SemaCXX/warn-unused-filescoped.cpp

void test() {
     template_test(x);
   }
 }
 
 namespace test4 {
   namespace { struct A {}; }
 
   void test(A a); // expected-warning {{unused function}}
-  extern "C" void test4(A a);
 }
 
 namespace rdar8733476 {
   static void foo() { } // expected-warning {{not needed and will not be emitted}}
 
   template <int>
   void bar() {
     foo();