Page MenuHomePhabricator

[CodeView] Emit function options for subprogram and member functions
ClosedPublic

Authored by asmith on Mar 31 2018, 8:24 AM.

Details

Summary

Use the newly added DebugInfo (DI) Trivial flag, which indicates if a C++ record is trivial or not, to determine Codeview::FunctionOptions.

Clang and MSVC generate slightly different Codeview for C++ records. For example, here is the C++ code for a class with a defaulted ctor,

class C {
public:
  C() = default;
};

Clang will produce a LF for the defaulted ctor while MSVC does not. For more details, refer to FIXMEs in the test cases in "function-options.ll" included with this set of changes.

Diff Detail

Repository
rL LLVM

Event Timeline

asmith created this revision.Mar 31 2018, 8:24 AM
asmith edited the summary of this revision. (Show Details)Mar 31 2018, 8:24 AM
rnk added a comment.Apr 10 2018, 3:09 PM

I think we actually want the triviality marker on the DISubprogram, not the DICompositeType. If I understand, this patch will still emit LF_METHOD records for trivial, but user-defaulted constructors, like:

struct Foo { Foo() = default; };

There's no LF_METHOD generated by Clang. Take a look at C1Class in the included tests which is the same are your example.

class C1Class {
public:
  C1Class() = default; // FIXME: Clang generates defaulted ctor while MSVC doesn't.
                       // We won't check what clang yields in this test. (though FO is correct)
};
rnk added a comment.Apr 11 2018, 9:22 AM

You're right, this patch does get the constructor member LF right, but it would be easier and more direct if the constructor itself were marked trivial or just omitted by clang.

I'll ask Reid to take another look at this next week. He's been on a long vacation.

rnk added a comment.Jul 20 2018, 2:27 PM

It might be best to limit the scope of the patch to just CxxReturnUdt and we can fix the other flags incrementally.

CodeGen/AsmPrinter/CodeViewDebug.cpp
334 ↗(On Diff #140543)

This isn't going to work. The frontend is going to need to tag classes with the information you want. Consider this example:

struct A {};
struct B : virtual A {
  B();
};
struct C : B {
  C();
};
C::C() {}

Clang will only emit a forward declaration for B, so you won't be able to walk the whole class hierarchy.

373 ↗(On Diff #140543)

For constructors, what's the purpose of checking if the class is trivial?

Hui added a subscriber: Hui.Jul 24 2018, 3:19 PM
Hui added inline comments.
CodeGen/AsmPrinter/CodeViewDebug.cpp
373 ↗(On Diff #140543)

It is observed that MSVC does not emit FunctionOptions::Constructor for a trivial ctor. Checking triviality of ctor is to conform with MSVC.

Hui added inline comments.Jul 24 2018, 4:26 PM
CodeGen/AsmPrinter/CodeViewDebug.cpp
334 ↗(On Diff #140543)

Yes. You are right. However it depends on whether Clang would like to emit complete type for B or not. This routine is transparent to incomplete/complete type of UDT.

clang only emts forwarded B.

Struct (0x1001) {
   TypeLeafKind: LF_STRUCTURE (0x1505)
   MemberCount: 0
   Properties [ (0x280)
     ForwardReference (0x80)
     HasUniqueName (0x200)
   ]
   FieldList: 0x0
   DerivedFrom: 0x0
   VShape: 0x0
   SizeOf: 0
   Name: B
   LinkageName: .?AUB@@
 }

MSVC emits both.

Struct (0x1001) {
   TypeLeafKind: LF_STRUCTURE (0x1505)
   MemberCount: 0
   Properties [ (0x280)
     ForwardReference (0x80)
     HasUniqueName (0x200)
   ]
   FieldList: 0x0
   DerivedFrom: 0x0
   VShape: 0x0
   SizeOf: 0
   Name: B
   LinkageName: .?AUB@@
 }

 Struct (0x101A) {
   TypeLeafKind: LF_STRUCTURE (0x1505)
   MemberCount: 6
   Properties [ (0x226)
     HasConstructorOrDestructor (0x2)
     HasOverloadedAssignmentOperator (0x20)
     HasOverloadedOperator (0x4)
     HasUniqueName (0x200)
   ]
   FieldList: <field list> (0x1019)
   DerivedFrom: 0x0
   VShape: 0x0
   SizeOf: 8
   Name: B
   LinkageName: .?AUB@@
 }
rnk added inline comments.Jul 27 2018, 11:37 AM
CodeGen/AsmPrinter/CodeViewDebug.cpp
334 ↗(On Diff #140543)

Yes, this is by design. In order to get this right 100% of the time, we will need a new flag, and this code will not be needed. I would prefer not to add it in the first place. Please remove this and reduce the scope of the patch. You can leave a FIXME comment about setting FunctionOptions::ConstructorWithVBase later.

asmith updated this revision to Diff 167666.Sep 30 2018, 3:07 PM

Limit the scope of the patch per feedback from RNK

rnk accepted this revision.Oct 1 2018, 1:54 PM

lgtm, thanks!

test/DebugInfo/COFF/function-options.ll
71 ↗(On Diff #167666)

In the future, you might find llvm-pdbutil dump -types to be a little more FileCheck-friendly. It's more concise, and it works on both object files and PDBs, so you only have to read one dumper format.

This revision is now accepted and ready to land.Oct 1 2018, 1:54 PM
This revision was automatically updated to reflect the committed changes.