[PATCH] D45123: [CodeView] Emit function options for subprogram and member functions

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: CodeGen/AsmPrinter/CodeViewDebug.cpp:334
+
+static bool hasVirtualBase(const DICompositeType *DCTy) {
+  for (auto *Element : DCTy->getElements()) {
----------------
Hui wrote:
> rnk wrote:
> > 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.
> 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@@
>   }
> ```
> 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D45123