[PATCH] [ms-cxxabi] Get closer to emitting the correct set of destructors in each TU (PR12784).
Timur Iskhodzhanov
timurrrr at google.com
Mon May 6 06:04:17 PDT 2013
[Let's continue on the phabricator review thread]
2013/4/27 Peter Collingbourne <peter at pcc.me.uk>:
> On Fri, Apr 26, 2013 at 06:45:04PM +0400, Timur Iskhodzhanov wrote:
>> Hi Peter,
>>
>> lib/AST/MicrosoftMangle.cpp
>> @@ -551,17 +551,18 @@ void
>> MicrosoftCXXNameMangler::manglePostfix(const DeclContext *DC,
>> void MicrosoftCXXNameMangler::mangleCXXDtorType(CXXDtorType T) {
>> switch (T) {
>> case Dtor_VectorDeleting:
>> Out << "?_E";
>> return;
>> case Dtor_Deleting:
>> Out << "?_G";
>> return;
>> case Dtor_Base:
>> Out << "?1";
>> return;
>> case Dtor_Complete:
>> Out << "?_D";
>> return;
>> }
>> --
>> Why do you think Complete is "?_D" and Base is "?1" ?
>> I thought "?1" stands for the "usual" destructor (complete) whilst
>> "?_D" stands for vbase destructor?
>
> The term "vbase destructor" is somewhat confusing -- it is in fact most
> similar to the complete destructor in the Itanium ABI. Please consider the
> following TU:
>
> ------------------------------------------------------------------------
> struct A {
> ~A();
> };
>
> struct B : virtual A {
> ~B();
> };
>
> void f(B *b) {
> b->~B();
> }
> ------------------------------------------------------------------------
>
> cl.exe's function f will (indirectly) call '?_DB' which in turn calls
> the base destructors '?1B' and '?1A'.
Agree.
>> See test below.
>>
>> test/CodeGenCXX/microsoft-abi-static-initializers.cpp
>> @@ -11,7 +11,7 @@ struct S {
>> // CHECK: ret void
>>
>> // CHECK: define internal void @"__dtor_\01?s@@3US@@A"() [[NUW]] {
>> -// CHECK: call x86_thiscallcc void @"\01??1S@@QAE at XZ"
>> +// CHECK: call x86_thiscallcc void @"\01??_DS@@QAE at XZ"
>> // CHECK: ret void
>> ---
>> I believe this is wrong.
>> CL does not emit a "??_DS@@" destructor on this file and it calls
>> "??1S@@..." when "s" is to be destroyed.
>
> This would appear to be an optimisation similar to the one we perform
> when we know that the object we are generating a destructor for does not
> have virtual bases, in which case we know that the base destructor can
> be used directly (in Itanium we emit an alias, cl.exe just calls it).
It would be nice to follow that logic.
>> lib/AST/VTableBuilder.cpp
>> @@ -1951,7 +1951,7 @@ void VTableBuilder::dumpLayout(raw_ostream& Out) {
>> if (IsComplete)
>> Out << "() [complete]";
>> else if (isMicrosoftABI())
>> - Out << "() [scalar deleting]";
>> + Out << "() [vector deleting]";
>> else
>> Out << "() [deleting]";
>> ---
>> Hm, why do you think we should put a vector deleting dtor in the vftable?
>
> This is an ABI requirement -- cl.exe will emit a call of the vector
> deleting dtor in the vftable when it compiles a 'delete[]' of pointer
> to class type.
>
>> Please look at http://llvm.org/bugs/show_bug.cgi?id=15058#c4 - if we
>> don't emit the deleting dtor definition, the linker error happens for
>> a scalar deleting dtor.
>
> That's probably because of the external weak alias from the vector
> to the scalar deleting dtor.
OK, I think I agree - we should put the vector dtor in the vftable,
not the scalar one.
>> I think the "scalar" dtor deletes one object whilst the "vector" dtor
>> deletes multiple objects (e.g. when used with "delete []").
>
> Not necessarily. The scalar/vector deleting dtors decide their
> behaviour based on the second parameter, an integer in which the
> 2 lowest bits are significant. The lower bit decides whether to
> deallocate memory, and the higher bit decides whether to treat the
> operand like an array allocated with 'new []'. The only difference
> between the scalar and vector deleting dtors is that the vector dtor
> assumes that the higher bit is never set, so it can omit the code to
> perform array deletion.
Ah, ok.
So the scalar dtor is used for destroying exactly one object;
whilst the vector dtor can be used to destroy one object (if the 2nd
bit is not set) and an array (if it is set).
>> So I don't think we should change what's printed in the
>> VTableBuilder::dumpLayout.
>>
>> The vector and scalar deleting dtors are usually aliases, right?
>> Are you sure the scalar dtor is an alias for a vector dtor, not vice versa?
>
> Yes. The alias is in place so that a translation unit can replace
> the scalar deleting dtor for dynamic class type T with its vector
> deleting dtor if it sees an allocation of the form 'new T[]'.
Agree.
>> lib/CodeGen/MicrosoftCXXABI.cpp
>> @@ -328,17 +331,18 @@ RValue
>> MicrosoftCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF,
>> ...
>> ASTContext &Context = CGF.getContext();
>> llvm::Value *ImplicitParam
>> - = llvm::ConstantInt::get(llvm::IntegerType::getInt1Ty(CGF.getLLVMContext()),
>> - DtorType == Dtor_Deleting);
>> + = llvm::ConstantInt::get(llvm::IntegerType::getInt32Ty(CGF.getLLVMContext()),
>> + DtorType == Dtor_Deleting);
>> ---
>> Are you sure there's a 32-bit argument? I'm sure I put Int1/bool on
>> purpose because I saw zero extension somewhere. Is there any case
>> where something other than 0/1 is passed as an argument to
>> scalar/vector deleting dtor?
>
> Yes, a 'delete[]' emitted by cl.exe will pass an argument of 3.
Agree
>> lib/CodeGen/CGCXX.cpp
>> @@ -230,15 +239,17 @@ CodeGenModule::GetAddrOfCXXConstructor(const
>> CXXConstructorDecl *ctor,
>> }
>>
>> void CodeGenModule::EmitCXXDestructors(const CXXDestructorDecl *D) {
>> ...
>> + if (getTarget().getCXXABI().hasDestructorVariants()) {
>> ---
>> Hm, is there any C++ ABI which doesn't have destructor variants? :)
>
> I should probably change this code. I guess it would in
> principle be possible to design an ABI where the choice between
> base/complete/deleting dtors is made based on a parameter.
Yes, but it's the wrong condition to use for MS ABI :)
>> 2013/4/26 Peter Collingbourne <peter at pcc.me.uk>:
>> > Hi Timur,
>> >
>> > Patch attached -- please review.
>> >
>> > The main missing part of this patch is vector deleting
>> > destructors, which we don't need ourselves but may be
>> > required by MSVC TUs.
>> Do you have an example when we really need them?
>>
>> I think they are only needed for polymorphic "operator delete[]",
>> which stackoverflow says to be an undefined behavior:
>> http://stackoverflow.com/questions/6171814/why-is-it-undefined-behavior-to-delete-an-array-of-derived-objects-via-a-base
>>
>> If that's just "nice to have", I'd personally like to defer that until
>> we have a justification to use them and real-world example of code
>> using it. Otherwise we don't even know if we implement it correctly :)
>
> I think we have to emit vector deleting dtors to maintain ABI compatibility
> with cl.exe, as otherwise 'delete[]' in a cl.exe compiled TU may break. For
> example, consider these two TUs:
>
> Compiled with Clang:
> ------------------------------------------------------------------------
> struct S {
> virtual ~S();
> };
>
> void f(S *s);
>
> int main(void) {
> S *s = new S[2];
> f(s);
> }
> ------------------------------------------------------------------------
>
> Compiled with cl.exe:
> ------------------------------------------------------------------------
> struct S {
> virtual ~S();
> };
>
> void f(S *s) {
> delete[] s;
> }
> ------------------------------------------------------------------------
>
> As mentioned above, 'delete[] s' will call the deleting dtor in the
> vftable. Unless we emit a vector deleting dtor as a result of seeing
> 'new S[2]' in our TU, the scalar deleting dtor will be called, and
> only one instance of S will be deleted (and the wrong memory address
> will be passed to 'operator delete[]()').
That'd happed even if the delete[] is not polymorphic, right?
Oh...
> Thanks,
> --
> Peter
More information about the cfe-commits
mailing list