[ItaniumCXXABI] Always use linkonce_odr linkage for RTTI data on MinGW

[mstorsjo]

Summary

This fixes cases where dynamic classes produced RTTI data with external linkage, producing linker errors about duplicate symbols.

This touches code close to what was changed in SVN r244266, but this change doesn't break the tests added in that revision.

I have to admit that I have little clue about what I'm doing here and I'm not sure if the exact fix is correct (it feels very heavy handed).

Hopefully the testcase shows what this fixes at least; this fixes linker errors with pretty trivial setups, where one object file contains the definition of a class with a virtual method (making the class dynamic, which previously meant that the typeinfo was emitted with external linkage) and another object file references typeid(ThatClass) (which produced a linkonce_odr version of that class' typeinfo).

[mstorsjo]

The previous diff lacked the extra context around the diff - fixed.

[majnemer]

LGTM

[mstorsjo]

lib/CodeGen/ItaniumCXXABI.cpp

2994 ↗

(On Diff #113309)

Thanks for having a look at this!

When giving this a second thought now, should we perhaps skip the check for Environment == GNU altogether? Or should xxx-windows-itanium be different by restricting this behaviour strictly to GNU (does it matter)?

And given the existing test case that you added with the previous form of this code, and the new test I'm adding here, are there any other odd corner cases I should check?

[mstorsjo]

lib/CodeGen/ItaniumCXXABI.cpp

2994 ↗

(On Diff #113309)

To rephrase; previously, the comment talked about mingw specifically, but the code only checked for the dllimport attribute. So perhaps I should drop the check for GNU and just check for windows?

[compnerd]

lib/CodeGen/ItaniumCXXABI.cpp

2990–2993 ↗

(On Diff #113309)

This is equivalent to: CGM.getContext().getTargetInfo().getTripleInfo().isWindowsGNUEnvironment()

2994 ↗

(On Diff #113309)

Im not 100% certain off the top of my head, but I believe that windows-itanium should actually import the typename, so this should be okay.

[mstorsjo]

lib/CodeGen/ItaniumCXXABI.cpp

2990–2993 ↗

(On Diff #113309)

Oh, indeed. Will change that before pushing. Thanks!

2994 ↗

(On Diff #113309)

Ok then, keeping it specific to MinGW..

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

case ExternalLinkage:
     if (const RecordType *Record = dyn_cast<RecordType>(Ty)) {
       const CXXRecordDecl *RD = cast<CXXRecordDecl>(Record->getDecl());
       if (RD->hasAttr<WeakAttr>())
         return llvm::GlobalValue::WeakODRLinkage;
       if (CGM.getTriple().isWindowsItaniumEnvironment())
         if (RD->hasAttr<DLLImportAttr>() &&
             ShouldUseExternalRTTIDescriptor(CGM, Ty))
           return llvm::GlobalValue::ExternalLinkage;
-      if (RD->isDynamicClass()) {
-        llvm::GlobalValue::LinkageTypes LT = CGM.getVTableLinkage(RD);
-        // MinGW won't export the RTTI information when there is a key function.
-        // Make sure we emit our own copy instead of attempting to dllimport it.
-        if (RD->hasAttr<DLLImportAttr>() &&
-            llvm::GlobalValue::isAvailableExternallyLinkage(LT))
-          LT = llvm::GlobalValue::LinkOnceODRLinkage;
-        return LT;
-      }
+      // MinGW always uses LinkOnceODRLinkage for type info.
+      if (RD->isDynamicClass() &&
+          !CGM.getContext()
+               .getTargetInfo()
+               .getTriple()
+               .isWindowsGNUEnvironment())
+        return CGM.getVTableLinkage(RD);
     }
 
     return llvm::GlobalValue::LinkOnceODRLinkage;
   }
 
   llvm_unreachable("Invalid linkage!");
 }
 

cfe/trunk/test/CodeGenCXX/rtti-mingw64.cpp

 // RUN: %clang_cc1 -triple x86_64-windows-gnu %s -emit-llvm -o - | FileCheck %s
 struct A { int a; };
 struct B : virtual A { int b; };
 B b;
+class C {
+  virtual ~C();
+};
+C::~C() {}
 
+// CHECK: @_ZTI1C = linkonce_odr
 // CHECK: @_ZTI1B = linkonce_odr constant { i8*, i8*, i32, i32, i8*, i64 }
 // CHECK-SAME:  i8* bitcast (i8** getelementptr inbounds (i8*, i8** @_ZTVN10__cxxabiv121__vmi_class_type_infoE, i64 2) to i8*),
 // CHECK-SAME:  i8* getelementptr inbounds ([3 x i8], [3 x i8]* @_ZTS1B, i32 0, i32 0),
 // CHECK-SAME:  i32 0,
 // CHECK-SAME:  i32 1,
 // CHECK-SAME:  i8* bitcast ({ i8*, i8* }* @_ZTI1A to i8*),
 //    This i64 is important, it should be an i64, not an i32.
 // CHECK-SAME:  i64 -6141 }, comdat