Page MenuHomePhabricator

[MS-ABI]V-base dtor called more than needed when throw happens in v-base ctor in window. Need add "complete object flag" check in eh cleanup code.
ClosedPublic

Authored by jyu2 on Dec 2 2016, 11:29 AM.

Details

Summary

The problem only happen on window ( A MS-ABI issuer )

The nature of the problem is virtual base dtor called more than it is needed after exception throw in inheriting base class(with virtual bases) ctor.

The root problem is when throw happen, not all virtual base classes have been contructed, so not all virtual base dtors are need to call for ehcleanup.

clang has code to handle vbase initialization: basically add check for "complete object flag" before call to v-base ctor.
But that part is missing for cleanup code.

To fix this add similar code as v-base init to cleanup code, same algorithm.

1> Add new routine:
EmitDtorCompleteObjectHandler

With corresponding to EmitCtorCompleteObjectHandler

2> In the EmitDestructorCal
Call EmitDtorCompleteObjectHandler when generate ehcleanup inside ctor.

Just add check for "complete object flag" before call to v-base dtor.

Without my change:
ehcleanup: ; preds = %ctor.skip_vbases

%13 = cleanuppad within none [], !dbg !66
%14 = bitcast %struct.class_0* %this1 to i8*, !dbg !66
%15 = getelementptr inbounds i8, i8* %14, i64 8, !dbg !66
%16 = bitcast i8* %15 to %struct.class_2*, !dbg !66
call void @"\01??1class_2@@UEAA@XZ"(%struct.class_2* %16) #6 [ "funclet"(token

%13) ], !dbg !66

cleanupret from %13 unwind to caller, !dbg !66

with my change:
ehcleanup: ; preds = %ctor.skip_vbases

%13 = cleanuppad within none [], !dbg !66
%14 = bitcast %struct.class_0* %this1 to i8*, !dbg !66
%15 = getelementptr inbounds i8, i8* %14, i64 8, !dbg !66
%16 = bitcast i8* %15 to %struct.class_2*, !dbg !66
%is_complete_object4 = icmp ne i32 %is_most_derived2, 0, !dbg !66
br i1 %is_complete_object4, label %Dtor.dtor_vbase, label %Dtor.skip_vbase, !d

bg !66

Dtor.dtor_vbase: ; preds = %ehcleanup

call void @"\01??1class_2@@UEAA@XZ"(%struct.class_2* %16) #6 [ "funclet"(token

%13) ], !dbg !66

br label %Dtor.skip_vbase, !dbg !66

Dtor.skip_vbase: ; preds = %Dtor.dtor_vbase, %ehcleanup

cleanupret from %13 unwind to caller, !dbg !66

Please let me know you need more info.

Diff Detail

Repository
rL LLVM

Event Timeline

jyu2 updated this revision to Diff 80097.Dec 2 2016, 11:29 AM
jyu2 retitled this revision from to V-base dtor called more than needed when throw happens in v-base ctor in window. Need add "complete object flag" check in eh cleanup code..
jyu2 updated this object.
jyu2 added reviewers: cfe-commits, erichkeane, majnemer, rnk.
jyu2 set the repository for this revision to rL LLVM.
jyu2 added a project: Restricted Project.
jyu2 retitled this revision from V-base dtor called more than needed when throw happens in v-base ctor in window. Need add "complete object flag" check in eh cleanup code. to [MS-ABI]V-base dtor called more than needed when throw happens in v-base ctor in window. Need add "complete object flag" check in eh cleanup code..Dec 4 2016, 11:29 AM
jyu2 updated this object.
rnk edited edge metadata.Dec 6 2016, 10:07 AM

This highlights an interesting MS/Itanium C++ ABI difference. GCC and Clang emit cleanup landing pads in destructors to ensure that all subobjects are destroyed even if one throws an exception. In Mingw, both GCC and Clang print ~A and ~B for this program, but MSVC does not:

extern "C" int puts(const char *);
struct A { virtual ~A() noexcept(false) { puts("~A"); } };
struct B { virtual ~B() noexcept(false) { puts("~B"); } };
struct C : virtual A, virtual B { virtual ~C() noexcept(false) { throw 1; } };
int main() {
  try {
    C c;
  } catch (...) {
    puts("caught");
  }
}

I encountered this while tracing out call sites in clang where ForVirtualBase is set to true in EmitCXXDestructorCall.

lib/CodeGen/MicrosoftCXXABI.cpp
1530–1532 ↗(On Diff #80097)

These checks seem unnecessary. ForVirtualBase should never be true if there are no vbases, and the IsMostDerivedClass assert will catch it if not.

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
285 ↗(On Diff #80097)

Most of these can be declarations to reduce the output size.

316 ↗(On Diff #80097)

Check for a call to the class_2 destructor here

jyu2 updated this revision to Diff 80459.Dec 6 2016, 11:47 AM
jyu2 edited edge metadata.
jyu2 removed rL LLVM as the repository for this revision.

Update patch to address Reid's suggestion

jyu2 added a comment.EditedDec 6 2016, 11:50 AM

Hi Reid,

I know some problems(ms compatibility) when throw inside destructor. I have not yet look those problems. I am new for clang. I need sometime to catch up. -:)

Thank you so much for your code review. I had add new patch to address your suggestion.

Please take look. Let me know if you need more info.

Thanks.
Jennifer

lib/CodeGen/MicrosoftCXXABI.cpp
1530–1532 ↗(On Diff #80097)

Yes, you are right. I can either check here, or check if IsMostDerivedClass is nullptr return instead assertion inside EmitDtorCompleteObjectHandler.

As you know ForVirutalBase is set also for destructor. But we only need this for ctor.

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
285 ↗(On Diff #80097)

Thank you! I changed, see my new diff

316 ↗(On Diff #80097)

Thank you so much. I changed see my new diff.

jyu2 updated this revision to Diff 80462.Dec 6 2016, 12:05 PM
jyu2 set the repository for this revision to rL LLVM.

A typo.

rnk added a comment.Dec 6 2016, 1:54 PM

Looks pretty good, just one more thing.

lib/CodeGen/MicrosoftCXXABI.cpp
1530–1532 ↗(On Diff #80097)

Yes, the first if check is necessary, but the second check for ClassDecl != nullptr && ClassDecl->getNumVBases() should never be false when ForVirtualBase is true.

jyu2 added a comment.Dec 6 2016, 2:33 PM

I changed! Thank you do much. Upload new patch.

lib/CodeGen/MicrosoftCXXABI.cpp
1530–1532 ↗(On Diff #80097)

Yes, make sense!! I don't know what I was think. Changed.

jyu2 updated this revision to Diff 80487.Dec 6 2016, 2:35 PM

Remove unnecessary check.

jyu2 updated this revision to Diff 80491.Dec 6 2016, 2:50 PM

last one missing test part diff. Sorry for that

rnk accepted this revision.Dec 6 2016, 4:22 PM
rnk edited edge metadata.

Looks good, please commit. If you don't have commit access, ask a coworker who does to land it, otherwise I'll try to get around to it at some point. Thanks for the fix!

This revision is now accepted and ready to land.Dec 6 2016, 4:22 PM
erichkeane edited edge metadata.Dec 6 2016, 4:23 PM

I'll land it, thanks!

This revision was automatically updated to reflect the committed changes.