dblaikie 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;
+ }
----------------
akhuang wrote:
> 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.
> 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)
Yeah, fair point - that'd be a nice generalization. (indirectly, you mean, for
instance - ctor of A takes a reference to B that contains an A member?).
> I think we can replace the loop with return `!RD->isAggregate() &&
> !RD->hasTrivialDefaultConstructor();`.
OK - so any type with a user-defined ctor would be a non-aggregate, we don't
care about the copy/move ctors so the only possibly trivial ctor that could
sneak through would be the default one. Hmm - what cases would we miss by
/only/ testing that the type doesn't have a trivial default ctor? Ah, right,
the stuff we've just been discussing where the ctor isn't instantiated... and
the !aggregate test will cover us there? OK, think I'm with it & really
appreciate the nuance.
> (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.)
Yeah, I take it that's valid/not UB? Figured there was that tiny case & yeah,
I'm willing to let that slide/through, really.
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:36-56
+
+// CHECK-DAG: ![[G:.*]] ={{.*}}!DICompositeType({{.*}}name:
"G"{{.*}}DIFlagTypePassByValue
+// CHECK-DAG: !DICompositeType({{.*}}scope: ![[G]], {{.*}}DIFlagTypePassByValue
+struct G {
+ G() : g_(0) {}
+ struct {
+ int g_;
----------------
Probably good to have a comment on each of these tests to clarify their purpose.
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:46-50
+// CHECK-DAG: !DICompositeType({{.*}}name: "H",{{.*}}DIFlagTypePassByValue
+struct H {
+ B b;
+};
+void f(H h) {}
----------------
This is to test an implicit, non-trivial default ctor of H?
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:52-56
+// CHECK-DAG: !DICompositeType({{.*}}name: "I",{{.*}}DIFlagTypePassByValue
+struct I {
+ B b;
+};
+void f(decltype(I()) i) {}
----------------
Looks like another implicit non-trivial ctor? Oh, I guess the previous case is
an implicit non-trivial default ctor that isn't instantiated/built? (so the
implementation doesn't observe any default ctor in 'I' at all?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87808/new/
https://reviews.llvm.org/D87808
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
