[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.


================
Comment at: lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py:28
+# CHECK:           |-CXXConstructorDecl {{.*}} Wrapper 'void ()'
+# CHECK-NEXT:      | `-AsmLabelAttr {{.*}} Implicit "_ZN7WrapperI3FooEC1B4testEv"
+# CHECK-NEXT:      `-CXXDestructorDecl {{.*}} ~Wrapper 'void ()'
----------------
labath wrote:
> Michael137 wrote:
> > Michael137 wrote:
> > > labath wrote:
> > > > I think this could be a bit of a problem, because (as you've probably found out by now) there are multiple versions of a single constructor, and the asm label seems to cause clang to coalesce them. In the simple test case below that doesn't matter, as the two constructors are identical, but things might be different if the class had virtual bases. (i.e., it could cause us to call the wrong constructor and blow up).
> > > I may be misunderstanding, but wouldn't they just get added as extra `CXXConstructorDecl`s on the AST with distinct `AsmLabel`s? Each constructor subprogram DIE links to some specification, which is the definition of the constructor we should call. That's where we get the linkage name from. Playing around with virtual bases I didn't yet manage to come up with a counterexample of where we would pick the wrong constructor
> > > 
> > > Although I did now notice that there's an extra destructor call in some cases where I didn't expect one before. Maybe that's a manifestation of the issue you describe. Investigating...
> > You probably mean multiple constructor definitions can point to the same specification DIE? In which case we would choose the first one happen to see. To what extent can these definitions differ?
> I mean the "subobject" and "full" constructor (and destructor) variants. There is only a single DIE for the constructor (presumably, because there is only one constructor at the source level), but there are two functions in the object.
> 
> You can try the following program to get an idea of what's happening:
> ```
> struct A {
>   A(int) {}
>   A(float) {}
> };
> 
> struct B : virtual A {
>   B();
> };
> B::B() : A(47) {}
> 
> struct C : B {
>   C() : A(4.7f) {}
> };
> 
> int main() {
>   B b;
>   C c;
> }
> ```
> In this program, the `b` constructor calls the full object constructor for `B` (`_ZN1BC1Ev`), whereas the B object constructed as a part of the `c` full object calls the base object constructor for `B` (`_ZN1BC2Ev`).
Ok that's very useful, thanks!
That does indeed look problematic. Will again revert for now. If we want to go down the linkage-name path I'd have to see if there's more context in DWARFASTParserClang that we can use to filter down to the correct definition. Maybe there's a better way to get these symbols resolved. Don't think the fallback to a guessed mangled name can save us here though


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143652/new/

https://reviews.llvm.org/D143652