Fix the fact that we don't assign profile counters to constructors in
classes with virtual bases, or constructors with variadic parameters.
I checked that we get a proper code coverage report for both tests.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/CodeGen/CodeGenPGO.cpp | ||
|---|---|---|
| 631 ↗ | (On Diff #89006) | I think it would be more appropriate to use the IsConstructorDelegationValid static function from CGClass.cpp instead of using the numVBases check, as some virtual inheritance constructors might get delegated in the future (as stated in the comments inside of IsConstructorDelegationValid). |
Comment Actions
- Use IsConstructorDelegationValid per Alex's suggestion. This exposed the variadic constructor case which we had also missed previously.
Comment Actions
LGTM.
One point to note, when we are displaying coverage for constructors that have both the base and complete versions instrumented, e.g.:
class Foo {
public:
Foo() { }
};
class Bar : virtual public Foo {
public:
Bar() { }; // llvm-cov will show 2 instantiations for Bar()
};
class BarBar: public Bar {
public:
BarBar() { }
};
int main (int argc, char* argv[])
{
BarBar b;
Bar bb;
}llvm-cov will treat each constructor as an instantiation (like a template instantiation). Should they be treated as instantiations though?
Comment Actions
No, ideally we'd combine the coverage reporting for Bar's constructors, since it's unlikely that users care about the distinction between them:
/tmp/x.cc:
1| |class Foo {
2| |public:
3| 2| Foo() { }
4| |};
5| |
6| |class Bar : virtual public Foo {
7| |public:
8| 2| Bar() { }; // llvm-cov will show 2 instantiations for Bar()
------------------
| _ZN3BarC2Ev: //< It doesn't help that these symbols demangle to the same name..
| 8| 1| Bar() { }; // llvm-cov will show 2 instantiations for Bar()
------------------
| _ZN3BarC1Ev:
| 8| 1| Bar() { }; // llvm-cov will show 2 instantiations for Bar()
------------------
9| |};
10| |
11| |class BarBar: public Bar {
12| |public:
13| 1| BarBar() { }
14| |};
15| |
16| |int main (int argc, char* argv[])
17| 1|{
18| 1| BarBar b; // calls _ZN6BarBarC1Ev -> {_ZN3FooC2Ev, _ZN3BarC2Ev} (leaves)
19| 1| Bar bb; // calls _ZN3BarC1Ev -> _ZN3FooC2Ev (leaf)
20| 1|}I don't think this report is 'terrible' though. It's certainly better than saying "BarBar"/"Bar" are executed 0/1 times, which is the status quo.