Page MenuHomePhabricator

[CodeView] Emit HasConstructorOrDestructor class option for non-trivial constructors
ClosedPublic

Authored by asmith on Mar 12 2018, 6:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

asmith created this revision.Mar 12 2018, 6:02 PM
zturner added inline comments.Mar 13 2018, 10:28 AM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1769 ↗(On Diff #138116)

How does cl behave if you have copy constructor or move constructor? Does it set this flag then?

Also what does this flag really mean? Does it mean "has non-defaulted copy constructor or destructor?" Or "has user-defined copy-constructor or destructor?" What would it be set to in each of the following cases?

struct A {
  int x;
};

struct B {
  B() = default;
  int x;
};

struct C {
  C(const C &) {}
};

struct D {
  std::string S;
};

Can you confirm that with this patch, our behavior matches the behavior of cl in each of these cases? (There may be more cases to think of, but this is at least a good starting point).

rnk added inline comments.Mar 13 2018, 1:32 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1772 ↗(On Diff #138116)

Eventually I think we'll need to mark constructors with FunctionOptions::Constructor, so we might as well go ahead and claim a bit from DIFlags for it now and add DISubprogram::isCXXConstructor. We can add an isCXXDestructor method that does the ~ check for now, since that doesn't require looking at the containing class type.

It doesn't look like DEFAULT special member functions (ctor/dtor/copy ctor/move ctor) are recorded in CodeView. For manually added SMFs, their names are the same as the class name. This lets us use Name == Ty->getName() to determine if it’s a ctor/move ctor or copy ctor.

asmith added inline comments.Mar 13 2018, 8:17 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1769 ↗(On Diff #138116)

struct A {

int x;            // Will have compiler generated SMF ctor/dtor

};

struct B {

B() = default;    // Using default (compiler generated) SMF ctor/dtor
int x;

};

struct C {

C(const C &) {}   // User defined SMF copy ctor

};

struct D {

std::string S;    // Will have compiler generated SMF ctor/dtor

};

zturner added a subscriber: asmith.Mar 13 2018, 8:32 PM

I know what kind of code the compiler will generate in each case, but I
don’t know off the top of my head what the CodeView will look like in each
case. Which ones have the HaveCinstructorOrDesrructor bit? Does your patch
match this behavior?

VS only sets HasConstructorOrDestructor for this example:

struct C {
  C(const C &) {}
};

Class (0x1007) {
    TypeLeafKind: LF_CLASS (0x1504)
    MemberCount: 1
    Properties [ (0x202)
      HasConstructorOrDestructor (0x2)
      HasUniqueName (0x200)
    ]
    FieldList: <field list> (0x1006)
    DerivedFrom: 0x0
    VShape: 0x0
    SizeOf: 1
    Name: CCC
    LinkageName: .?AVCCC@@
  }

Even for something like:

struct D {
  std::string S;
  D() = default;
};

?

Doesn't appear to be set for that example.

Struct (0x1E4A) {
   TypeLeafKind: LF_STRUCTURE (0x1505)
   MemberCount: 0
   Properties [ (0x280)
     ForwardReference (0x80)
     HasUniqueName (0x200)
   ]
   FieldList: 0x0
   DerivedFrom: 0x0
   VShape: 0x0
   SizeOf: 0
   Name: D
   LinkageName: .?AUD@@
rnk added a comment.Mar 14 2018, 10:00 AM

It sounds like the criteria for this flag is "has a non-trivial constructor". Here's two examples that show the difference:

struct Foo {
  Foo() = default;
  Foo(const Foo &o) = default;
  int m;
} f;
struct Bar {
  int m = 0;
} b;

Foo doesn't have the flag, even though it has two user-declared constructors, because they are trivial. Bar has the flag even though it has no user-declared constructors, because the default ctor must zero initialize the object.

Bar is interesting because Clang currently doesn't mark the "Bar" DICompositeType in any way that would let us set this flag correctly. It doesn't create a DISubprogram for Bar's non-trivial default ctor. MSVC does, so we might want to make Clang go out of its way to describe all the non-trivial special members of a class.

asmith retitled this revision from [CodeView] Emit HasConstructorOrDestructor class option to [CodeView] Emit HasConstructorOrDestructor class option for non-trivial constructors.Mar 18 2018, 12:02 PM

Can you add a test using the two examples rnk posted? After that this will be good.

Any other comments on this one?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 10:23 AM
zturner accepted this revision.Feb 7 2019, 10:29 AM
This revision is now accepted and ready to land.Feb 7 2019, 10:29 AM
rnk accepted this revision.Feb 7 2019, 10:55 AM

lgtm

Sorry, you posted the update while I was on vacation, and I declared inbox bankruptcy afterwards.

asmith updated this revision to Diff 188296.Feb 25 2019, 7:18 PM
This revision was automatically updated to reflect the committed changes.