[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

[Amy Huang via Phabricator via cfe-commits]

akhuang added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300
+  bool hasCtor = false;
+  for (const auto *Ctor : RD->ctors()) {
     if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
       return false;
+    if (!Ctor->isCopyOrMoveConstructor())
+      hasCtor = true;
+  }
----------------
rsmith wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > This looks pretty similar to:
> > > 
> > > ```
> > > return RD->hasUserDeclaredConstructor() && !RD->hasTrivialDefaultConstructor();
> > > ```
> > > 
> > > (other than its treatment of user-declared copy or move constructors), but I have to admit I don't really understand what the "at least one constructor and no trivial or constexpr constructors" rule aims to achieve, so it's hard to know if this is the right interpretation. The rule as written in the comment above is presumably not exactly right -- all classes have at least one constructor, and we're not excluding classes with trivial copy or move constructors, only those with trivial default constructors.
> > > 
> > > I wonder if the intent would be better captured by "at least one non-inline constructor" (that is, assuming all declared functions are defined, there is at least one translation unit in which a constructor is defined that can be used as a home for the class info).
> > So the general goal is to detect any type where the construction of that type somewhere must invoke a constructor that will be IR-generated.
> > 
> > Move and copy constructors are ignored because the assumption is they must be moving/copying from some other object, which must've been constructed, ultimately, by a non-move/copy constructor.
> > 
> > Ideally this would be usable even for inline ctors - even if the ctor calls get optimized away later[^1], they'd still allow us to reduce the number of places the type is emitted to only those places that call the ctor.
> > 
> > 
> > 
> > [^1] actually, the way that should work doesn't seem to be working right now (eg:
> > type.cpp
> > ```
> > struct t1 { t1() { } };
> > void f1(void*);
> > int main() {
> >   f1(new t1());
> > }
> > ```
> > type2.cpp
> > ```
> > struct t1 { t1() { } };
> > void f1(void* v) {
> >   delete (t1*)v;
> > }
> > ```
> > build: `clang++ type.cpp -g -Xclang -fuse-ctor-homing type2.cpp && llvm-dwarfdump a.out`
> > -> definition of "t1" in the DWARF
> > build with optimizations: `clang++ -O3 type.cpp -g -Xclang -fuse-ctor-homing type2.cpp && llvm-dwarfdump a.out`
> > -> missing definition of "t1"
> > `type.cpp` is chosen as a home for `t1` because it calls a user-defined ctor, but then that ctor gets optimized away and there's no other mention of `t1` in `type.cpp` so the type is dropped entirely. This could happen even with a non-inline definition - under LTO the ctor could get optimized away (though this would be safe in FullLTO - the other references to `t1` would be made to refer to the definition and keep it alive - but in ThinLTO the TU defining the ctor might be imported and nothing else - leaving the type unused/dropped)
> > To fix this we should put 'homed' types in the retained types list so they are preserved even if all other code/references to the type are dropped. I think I implemented this homed type pinning for explicit template specialization definitions, because they have no code attachment point, so similar logic could be used for ctor homing. (vtable homing /might/ benefit from this with aggressive/whole program devirtualization? Not sure - harder to actually optimize away all the references to a type, but possible maybe?)
> Oh, I see, that's clever and very neat.
> 
> So the intent is to identify types for which we know that either no instances are ever created, or some constructor must be actually emitted in some translation unit (prior to optimizations running). And that's why we want to ignore copy and move constructors [and could in fact ignore any constructor taking the class type by value or reference, directly or indirectly] (they cannot be the only place that creates an object) and also why we don't want to do this if there's a constexpr constructor or trivial default constructor (we might not generate code for them). And the "no constructors" check seems to effectively be checking whether aggregate initialization could be performed.
> 
> I think we can replace the loop with `return !RD->isAggregate() && !RD->hasTrivialDefaultConstructor();`.
> 
> (Minor aside, it is possible to create an instance of a type if its only constructor is a copy constructor, eg with `struct A { A(const A&) {} } a = a;`, but I doubt that's a practical concern for your purposes.)
> So the intent is to identify types for which we know that either no instances are ever created, or some constructor must be actually emitted in some translation unit (prior to optimizations running). And that's why we want to ignore copy and move constructors [and could in fact ignore any constructor taking the class type by value or reference, directly or indirectly] (they cannot be the only place that creates an object) and also why we don't want to do this if there's a constexpr constructor or trivial default constructor (we might not generate code for them). And the "no constructors" check seems to effectively be checking whether aggregate initialization could be performed.

> I think we can replace the loop with return !RD->isAggregate() && !RD->hasTrivialDefaultConstructor();.
Ok, I think that makes sense - I guess the loop was sort of attempting to do what `RD->hasTrivialDefaultConstructor()`does. 

> To fix this we should put 'homed' types in the retained types list so they are preserved even if all other code/references to the type are dropped.
Yeah, think you mentioned this before and then I never added it--I'll make a separate patch for it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87808