Fix for PR32990.

[zahiraam]

Summary

This fixes the bug in https://bugs.llvm.org/show_bug.cgi?id=32990.

[zahiraam]

Hello,

I would really appreciate if (one of) you can take time to review this patch?
Thanks in advance :)
-Zahira

[aaron.ballman]

This looks reasonable to me, but @rnk and @majnemer have more expertise in this area than I do.

[rnk]

lib/CodeGen/MicrosoftCXXABI.cpp

3803 ↗

(On Diff #119510)

typo on "virual".

Also, please use the Itanium nomenclature used here. The "complete" destructor is equivalent to the MS ABI vbase destructor. The "base" destructor is the one that demangles as just ~Foo, i.e. the one that starts with ??1Foo.

3807 ↗

(On Diff #119510)

Why did you include this here? It doesn't do anything at -O0, so I don't see how it fixes any correctness problems for you.

See this code:

bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
  if (!getCodeGenOpts().CXXCtorDtorAliases)
    return true;

  // Producing an alias to a base class ctor/dtor can degrade debug quality
  // as the debugger cannot tell them apart.
  if (getCodeGenOpts().OptimizationLevel == 0)
    return true;

test/CodeGenCXX/dllimport-base.cpp

7 ↗

(On Diff #119510)

This test case seems redundant with dllimport-dtor-thunks.cpp. I reduced it from this code originally.

test/CodeGenCXX/dllimport-dtor-thunks.cpp

25–26

Please update this comment to reflect your change.

test/CodeGenCXX/dllimport-virtual-base.cpp

68

These check-nots won't ever fire because you've placed this class into the test3 namespace.

[majnemer]

lib/CodeGen/MicrosoftCXXABI.cpp

3805–3807 ↗

(On Diff #119510)

Do we want this logic to kick in for dllimport for the Itanium ABI? I would think not. What does MinGW do?

[zahiraam]

Hello,

I have put another patch. I hope I have raised all your questions. Thank you for taking the time to review. Cheers. :-)

lib/CodeGen/MicrosoftCXXABI.cpp

3807 ↗

(On Diff #119510)

You are right it's not doing anything. Thanks.

test/CodeGenCXX/dllimport-base.cpp

7 ↗

(On Diff #119510)

Ok. I removed it all together.

[rnk]

lib/CodeGen/MicrosoftCXXABI.cpp

3806 ↗

(On Diff #120686)

Why check isWindowsMSVCEnvironment? That's implied by being in the MicrosoftCXXABI.cpp file. I think what you're trying to do here is to return early, so that we always use the imported complete or base destructors and never emit an inline version. Is that accurate? Have you considered emitting them as available_externally instead?

[zahiraam]

lib/CodeGen/MicrosoftCXXABI.cpp

3806 ↗

(On Diff #120686)

Yes that's accurate. I haven't tried. I will look into it.
Thanks.

[zahiraam]

Reid,

Let me know if that is what you meant.
Thanks.

[rnk]

I think after this change useThunkForDtorVariant won't be the right abstraction. The linkage and DLL storage class code in CodeGenModule.cpp will now need to handle all three destructor variant cases in the MS ABI differently, so we might as well remove it and sink the logic into CodeGenModule.cpp and switch on the destructor kind in both methods.

Here are the cases that I see:

	imported	non-imported	exported
base	fall through	fall through	fall through
complete	available_externally + dllimport	linkonce_odr	weak_odr + dllexport
deleting	linkonce_odr	linkonce_odr	linkonce_odr

Base destructors are always handled like normal function declarations, so we can just break out of the switch.

Deleting destructors seem to be different in every DLL to allow local operator delete overrides, so they are always linkonce_odr inline functions that are never imported.

Complete destructors are either linkonce_odr (non-imported) inline functions, available_externally dllimport inline functions, or weak_odr dllexport inline functions, depending on the storage class.

Does this make sense?

lib/CodeGen/CodeGenModule.cpp

847–856

This is checking the destructor type twice. The intention was to hide MS ABI specific details in MicrosoftCXXABI.cpp, but as you can see below, MS ABI knowledge of inheriting constructors has already leaked here, so perhaps this is a lost cause.

878–881

Won't you need to disable this logic so that we dllimport complete destructors?

test/CodeGenCXX/dllimport-dtor-thunks.cpp

45

I would've expected this to be defined as an available_externally dllimport function.

[zahiraam]

Yes I think it makes sense.
If I understand everything instead of testing is useThunkForDtorVariant is true we should switch one the type of destructor on both function member functions CodeGenModule::getFunctionLinkage and deGenModule::setFunctionDLLStorageClass following your logic above?

[rnk]

In D39063#914179, @zahiraam wrote:

Yes I think it makes sense.
If I understand everything instead of testing is useThunkForDtorVariant is true we should switch one the type of destructor on both function member functions CodeGenModule::getFunctionLinkage and deGenModule::setFunctionDLLStorageClass following your logic above?

Yep

[zahiraam]

Reid,

I implemented the logic above. Hope this is what you are expecting.
There is another use for that function that I think should also be eliminated. I am working on doing that. Meanwhile can you please look at the attached patch and let me know if it looks good.
Thanks.

[zahiraam]

Fixed a bug I found in previous version.

[rnk]

It looks like the new diff is missing the test cases, I was expecting to see dllimport available_externally vbase/complete destructors.

lib/CodeGen/CodeGenModule.cpp

878–881

What about this?

[zahiraam]

Reid,

Added the test cases.
Fixed the function to set the storage for complete type.
Removing the check isMS generated a bunch of errors. So not sure at this point if we can get rid of that condition.
If you are happy with this particular bug fix , then we can close this PR and I can continue working on removing the test to isMS ABI in this file? Let me know your thoughts.
Thanks.

[rnk]

I don't see any test cases looking for available_externally linkage. This looks like the interesting test case:

struct __declspec(dllimport) A { virtual ~A(); };
struct __declspec(dllimport) B : virtual A { virtual ~B(); };
void f() { B b; }

Compile this example in the MS ABI both with and without optimizations, and check for the declaration of B's vbase destructor for linkage and dll storage class.

lib/CodeGen/CodeGenModule.cpp

885–886

Don't we dllimport it if it has DLLImportAttr?

[zahiraam]

lib/CodeGen/CodeGenModule.cpp

847–856

I think I agree with you that this should be fixed. I propose that we close this PR (if you think the current patch fixes it) and open and new PR for the purpose of cleaning the MS ABI in this file. What do you think?

885–886

Yes that's true. I have updated that and that triggered a couple of problems in the test cases. I have fixed that too.

test/CodeGenCXX/dllimport-dtor-thunks.cpp

45

Yes with -O1. I added more testing to reflect that.

test/CodeGenCXX/dllimport-virtual-base.cpp

68

I fixed the test. I just removed those check-not all together. The rest of the check are enough.

[zahiraam]

Hello!

Would appreciate some feedback on this please, :-)
Thanks.

[rnk]

Looks good, sorry this sat at the bottom of my inbox so long! Thanks for the tests.

[zahiraam]

Thanks :)

[rnk]

I ended up having to revert this, the Chromium Windows build started failing: https://ci.chromium.org/buildbot/chromium.clang/ToTWin%28dbg%29/347

Errors look like:
[942/62751] LINK(DLL) common.dll common.dll.lib common.dll.pdb
FAILED: common.dll common.dll.lib common.dll.pdb
C:/b/depot_tools/win_tools-2_7_6_bin/python/bin/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x86 False link.exe /nologo /IMPLIB:./common.dll.lib /DLL /OUT:./common.dll /PDB:./common.dll.pdb @./common.dll.rsp
LINK : ./common.dll not found or not built by the last incremental link; performing full link
gcm_messages.obj : error LNK2019: unresolved external symbol "declspec(dllimport) public: void thiscall std::_Lockit::`vbase destructor'(void)" (imp_??_D_Lockit@std@@QAEXXZ) referenced in function "protected: void thiscall std::_Tree<class std::_Tmap_traits<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,struct std::less<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class std::allocator<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const ,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > >,0> >::_Orphan_ptr(struct std::_Tree_node<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const ,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,void *> *)" (?_Orphan_ptr@?$_Tree@V?$_Tmap_traits@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@U?$less@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@V?$allocator@U?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@@2@$0A@@std@@@std@@IAEXPAU?$_Tree_node@U?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@PAX@2@@Z)
./common.dll : fatal error LNK1120: 1 unresolved externals

So I strongly suspect this change. Let me know if you need help constructing a test case, I don't think it should be too hard to make one.

[zahiraam]

Yes it looks like it is due to this patch. Yes I wouldn't mind creating a test case. Thanks.

[zahiraam]

Can't seem to find a test case to break the build. Can you propose one please?
Thanks.

[zahiraam]

Reid,
Can you please suggest a test case to fix the build? I can't seem to find scenario to break it. Thanks.

[rnk]

Sorry, I haven't had time to put together a reproduction over the holidays. Looks like it affects STL classes.

[zahiraam]

In D39063#979580, @rnk wrote:

Sorry, I haven't had time to put together a reproduction over the holidays. Looks like it affects STL classes.

Yes I I figured from the error that was the STL (container) library. What do you suggest I do to reproduce the problem? Should I download the app and build it locally or do you think there is an easy way to reproduce a test case? I tried to do that using some container classes but couldn't create one that didn't compile?

[rnk]

Sorry for the delay, I came up with this reduction:

struct __declspec(dllimport) Foo {
  Foo() {}
  ~Foo() {}

  Foo(const Foo &) = delete;
  Foo &operator=(const Foo &) = delete;
};

void f() { Foo obj; }

After your change, if you compile this with clang at -O0, f will call Foo's vbase destructor, despite Foo not having any virtual bases. This will lead to link errors, since the DLL that exports Foo will not provide a definition of the vbase destructor.

[zahiraam]

[rnk]

I ended up putting together a patch that attempts to solve this issue: https://reviews.llvm.org/D44505
Does that cover all the cases here? I think we already have test for a lot of them in tree.

lib/CodeGen/CodeGenModule.cpp

675–676

There is still commented out code and the indentation seems off.

677

This doesn't seem right, since we definitely want to export base destructors (??1Foo@...).

lib/CodeGen/CodeGenModule.cpp

if (GV->hasLocalLinkage()) {
     return;
   }
 
   // Set visibility for definitions.
   LinkageInfo LV = D->getLinkageAndVisibility();
   if (LV.isVisibilityExplicit() || !GV->hasAvailableExternallyLinkage())
     GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
 static llvm::GlobalVariable::ThreadLocalMode GetLLVMTLSModel(StringRef S) {
   return llvm::StringSwitch<llvm::GlobalVariable::ThreadLocalMode>(S)
       .Case("global-dynamic", llvm::GlobalVariable::GeneralDynamicTLSModel)
       .Case("local-dynamic", llvm::GlobalVariable::LocalDynamicTLSModel)
       .Case("initial-exec", llvm::GlobalVariable::InitialExecTLSModel)
       .Case("local-exec", llvm::GlobalVariable::LocalExecTLSModel);
 }
 
 static llvm::GlobalVariable::ThreadLocalMode GetLLVMTLSModel(
     CodeGenOptions::TLSModel M) {

 
 llvm::GlobalValue::LinkageTypes
 CodeGenModule::getFunctionLinkage(GlobalDecl GD) {
   const auto *D = cast<FunctionDecl>(GD.getDecl());
 
   GVALinkage Linkage = getContext().GetGVALinkageForFunction(D);
 
   if (isa<CXXDestructorDecl>(D) &&
       getCXXABI().useThunkForDtorVariant(cast<CXXDestructorDecl>(D),
                                          GD.getDtorType())) {
     // Destructor variants in the Microsoft C++ ABI are always internal or
-    // linkonce_odr thunks emitted on an as-needed basis.
+    // linkonce_odr thunks emitted on an as-needed basis. Except destructors
+    // that have a virtual base and dllimport attributes. Those will
+    // be marked with an external linkage.
+    if (cast<CXXDestructorDecl>(D)->getParent()->getNumVBases() &&
+        (GD.getDtorType() == CXXDtorType::Dtor_Complete ||
+         GD.getDtorType() == CXXDtorType::Dtor_Base) &&
+        D->hasAttr<DLLImportAttr>())
+      return llvm::Function::AvailableExternallyLinkage;
+    else
       return Linkage == GVA_Internal ? llvm::GlobalValue::InternalLinkage
                                      : llvm::GlobalValue::LinkOnceODRLinkage;
   }
 
   if (isa<CXXConstructorDecl>(D) &&
       cast<CXXConstructorDecl>(D)->isInheritingConstructor() &&
       Context.getTargetInfo().getCXXABI().isMicrosoft()) {
     // Our approach to inheriting constructors is fundamentally different from
     // that used by the MS ABI, so keep our inheriting constructor thunks
     // internal rather than trying to pick an unambiguous mangling for them.
     return llvm::GlobalValue::InternalLinkage;
   }
 
   return getLLVMLinkageForDeclarator(D, Linkage, /*isConstantVariable=*/false);
 }
 
 void CodeGenModule::setFunctionDLLStorageClass(GlobalDecl GD, llvm::Function *F) {
   const auto *FD = cast<FunctionDecl>(GD.getDecl());
 
   if (const auto *Dtor = dyn_cast_or_null<CXXDestructorDecl>(FD)) {
     if (getCXXABI().useThunkForDtorVariant(Dtor, GD.getDtorType())) {
       // Don't dllexport/import destructor thunks.
       F->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
       return;
     }
   }
 
   if (FD->hasAttr<DLLImportAttr>())
     F->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
   else if (FD->hasAttr<DLLExportAttr>())
     F->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass);
   else
     F->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass);
 }
 
 llvm::ConstantInt *CodeGenModule::CreateCrossDsoCfiTypeId(llvm::Metadata *MD) {

test/CodeGenCXX/dllimport-dtor-thunks.cpp

 };
 
 // Non-virtually inherits from a non-dllimport base class. We should again call
 // the derived base constructor directly. No need for the complete (aka vbase)
 // destructor.
 struct __declspec(dllimport) ImportOverrideVDtor : public BaseClass {
   virtual ~ImportOverrideVDtor() {}
 };
 
-// Virtually inherits from a non-dllimport base class. This time we need to call
-// the complete destructor and emit it inline. It's not exported from the DLL,
-// and it must be emitted.
+// Virtually inherits from a non-dllimport base class. Emit the virtual base destructor.
 struct __declspec(dllimport) ImportVBaseOverrideVDtor
     : public virtual BaseClass {
   virtual ~ImportVBaseOverrideVDtor() {}
 };
 
 extern "C" void testit() {
   ImportIntroVDtor t1;
   ImportOverrideVDtor t2;
   ImportVBaseOverrideVDtor t3;
 }
 
 // The destructors are called in reverse order of construction. Only the third
 // needs the complete destructor (_D).
 // CHECK-LABEL: define void @testit()
 // CHECK:  call void @"\01??_DImportVBaseOverrideVDtor@@QEAAXXZ"(%struct.ImportVBaseOverrideVDtor* %{{.*}})
 // CHECK:  call void @"\01??1ImportOverrideVDtor@@UEAA@XZ"(%struct.ImportOverrideVDtor* %{{.*}})
 // CHECK:  call void @"\01??1ImportIntroVDtor@@UEAA@XZ"(%struct.ImportIntroVDtor* %{{.*}})
 
-// CHECK-LABEL: define linkonce_odr void @"\01??_DImportVBaseOverrideVDtor@@QEAAXXZ"
+// CHECK-LABEL: declare dllimport void @"\01??_DImportVBaseOverrideVDtor@@QEAAXXZ"
 // CHECK-LABEL: declare dllimport void @"\01??1ImportOverrideVDtor@@UEAA@XZ"
 // CHECK-LABEL: declare dllimport void @"\01??1ImportIntroVDtor@@UEAA@XZ"

test/CodeGenCXX/dllimport-virtual-base.cpp

+// RUN: %clang_cc1 -triple i386-pc-windows -emit-llvm -fms-compatibility %s -x c++ -o - | FileCheck %s
+
+namespace test1 {
+struct BaseClass {
+  ~BaseClass();
+};
+
+struct __declspec(dllimport) Concrete : virtual BaseClass {
+};
+
+Concrete c;
+} // namespace test1
+
+namespace test2 {
+class BaseClass {
+public:
+  virtual ~BaseClass(){};
+};
+
+class __declspec(dllimport) VirtualClass : public virtual BaseClass {
+public:
+  virtual ~VirtualClass(){};
+};
+
+int main() {
+  VirtualClass c;
+  return 0;
+}
+} // namespace test2
+
+namespace test3 {
+class IVirtualBase {
+public:
+  virtual ~IVirtualBase(){};
+  virtual void speak() = 0;
+};
+
+class VirtualClass : public virtual IVirtualBase {
+public:
+  virtual ~VirtualClass(){};
+  virtual void eat() = 0;
+};
+
+class __declspec(dllimport) ConcreteClass : public VirtualClass {
+public:
+  ConcreteClass(int nn);
+  void speak();
+  void eat();
+  virtual ~ConcreteClass();
+
+private:
+  int n;
+};
+
+int main() {
+  ConcreteClass c(10);
+  c.speak();
+  c.eat();
+  return 0;
+}
+} // namespace test3
+
+// CHECK-LABEL: declare dllimport x86_thiscallcc %"struct.test1::Concrete"* @"\01??0Concrete@test1@@QAE@XZ"
+// CHECK-LABEL: declare dllimport x86_thiscallcc %"class.test2::VirtualClass"* @"\01??0VirtualClass@test2@@QAE@XZ"
+// CHECK-LABEL: declare dllimport x86_thiscallcc %"class.test3::ConcreteClass"* @"\01??0ConcreteClass@test3@@QAE@H@Z"
+// CHECK-NOT: ??_DConcrete@test1@@QEAAXXZ
+// CHECK-NOT: ??_DVirtualClass@test2@@QEAAXXZ
+// CHECK-NOT: ??_DConcreteClass@test3@@QEAAXXZ