Page MenuHomePhabricator

Fix for PR32990.

Authored by zahiraam on Oct 18 2017, 11:22 AM.

Diff Detail

Event Timeline

zahiraam created this revision.Oct 18 2017, 11:22 AM
zahiraam edited the summary of this revision. (Show Details)Oct 19 2017, 3:22 PM
zahiraam added reviewers: aaron.ballman, erichkeane.


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

aaron.ballman edited edge metadata.
aaron.ballman added a subscriber: majnemer.

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

rnk added inline comments.Oct 25 2017, 1:35 PM

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.


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;
7 ↗(On Diff #119510)

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


Please update this comment to reflect your change.


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

majnemer added inline comments.Oct 26 2017, 2:23 PM

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

zahiraam updated this revision to Diff 120686.Oct 27 2017, 1:27 PM
zahiraam marked 5 inline comments as done.


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


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

7 ↗(On Diff #119510)

Ok. I removed it all together.

rnk added inline comments.Oct 27 2017, 2:36 PM

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 added inline comments.Oct 28 2017, 6:09 AM

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

zahiraam updated this revision to Diff 121175.Nov 1 2017, 1:23 PM
zahiraam marked 2 inline comments as done.


Let me know if that is what you meant.

rnk edited edge metadata.Nov 2 2017, 9:56 AM

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:

basefall throughfall throughfall through
completeavailable_externally + dllimportlinkonce_odrweak_odr + dllexport

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?


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.


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


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?

rnk added a comment.Nov 2 2017, 2:06 PM

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?


zahiraam updated this revision to Diff 121969.Nov 7 2017, 1:17 PM


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.

zahiraam updated this revision to Diff 122130.Nov 8 2017, 11:43 AM

Fixed a bug I found in previous version.

rnk added a comment.Nov 8 2017, 2:20 PM

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


What about this?

zahiraam updated this revision to Diff 122248.Nov 9 2017, 7:24 AM


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.

rnk added a comment.Nov 9 2017, 2:22 PM

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.


Don't we dllimport it if it has DLLImportAttr?

zahiraam updated this revision to Diff 122603.Nov 12 2017, 11:40 AM
zahiraam marked 4 inline comments as done.
zahiraam added inline comments.

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?


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


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


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


Would appreciate some feedback on this please, :-)

rnk accepted this revision.Dec 20 2017, 4:33 PM

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

This revision is now accepted and ready to land.Dec 20 2017, 4:33 PM
Closed by commit rL321239: Fix for PR32990 (authored by erichkeane, committed by ). · Explain WhyDec 20 2017, 6:08 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Dec 21 2017, 1:39 PM

I ended up having to revert this, the Chromium Windows build started failing:

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/ 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?

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

rnk added a comment.Jan 17 2018, 4:03 PM

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

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 added a comment.Mar 2 2018, 3:41 PM

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.

rnk added a comment.Mar 14 2018, 6:17 PM

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


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


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