This fixes the bug in https://bugs.llvm.org/show_bug.cgi?id=32990.
Details
Diff Detail
Event Timeline
Hello,
I would really appreciate if (one of) you can take time to review this patch?
Thanks in advance :)
-Zahira
lib/CodeGen/MicrosoftCXXABI.cpp | ||
---|---|---|
3824 | 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. | |
3828 | 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 | ||
26–27 | 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. |
lib/CodeGen/MicrosoftCXXABI.cpp | ||
---|---|---|
3826–3828 | Do we want this logic to kick in for dllimport for the Itanium ABI? I would think not. What does MinGW do? |
lib/CodeGen/MicrosoftCXXABI.cpp | ||
---|---|---|
3823–3824 | 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? |
lib/CodeGen/MicrosoftCXXABI.cpp | ||
---|---|---|
3823–3824 | Yes that's accurate. I haven't tried. I will look into it. |
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 | ||
---|---|---|
1094–1117 | 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. | |
1130–1131 | Won't you need to disable this logic so that we dllimport complete destructors? | |
test/CodeGenCXX/dllimport-dtor-thunks.cpp | ||
44 | I would've expected this to be defined as an available_externally dllimport function. |
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?
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.
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 | ||
---|---|---|
1130–1131 | What about this? |
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.
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 | ||
---|---|---|
1132–1133 | Don't we dllimport it if it has DLLImportAttr? |
lib/CodeGen/CodeGenModule.cpp | ||
---|---|---|
1094–1117 | 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? | |
1132–1133 | 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 | ||
44 | 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. |
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.
Yes it looks like it is due to this patch. Yes I wouldn't mind creating a test case. Thanks.
Can't seem to find a test case to break the build. Can you propose one please?
Thanks.
Reid,
Can you please suggest a test case to fix the build? I can't seem to find scenario to break it. Thanks.
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?
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.
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 | ||
---|---|---|
791 | There is still commented out code and the indentation seems off. | |
793 | This doesn't seem right, since we definitely want to export base destructors (??1Foo@...). |
There is still commented out code and the indentation seems off.