We need to emit barrier if the union field
is CXXRecordDecl because it might have vptrs. The testcode
was wrongly devirtualized. It also proves that having different
groups for different dynamic types is not sufficient.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | Checking for v-table pointers recursively isn't really that difficult, but if you really don't want to do that, please at least check for non-POD-ness or something so that this isn't kicking in for literally every struct type. |
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | ok, I am also planning to fix this in the later patch, because the same problem arise when comparing 2 pointers to dynamic classe. |
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | We could repeat this work from scratch on every single union field access done by IRGen and it would still be way, way faster than doing anything extra on every record definition in the program. The latter is done orders of magnitude more frequently than the former. |
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | This is a good point, and actually I don't need to check if class holds any vptrs for the example I refered (comparing 2 pointers). |
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | What are you planning to do if someone casts to a pointer to incomplete type? I am concerned that what these problems are really telling us is that this representation is not very good. |
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3517 ↗ | (On Diff #94569) | In this case we also need to add the barrier. I might be wrong, but I think this in general should be pretty rare and even more rare if it would limit the devirtualization. Derived *d; (Base*)d; will not limit devirtualization if Derived is complete type (because base will be also). The other thing is that with my recent patches, adding more barriers will not limit any memory optimizations. |
For now I will check if it has any vptrs. It will be probably soon stored in the CXXRecordDecl because it will be used much more frequently when generating barriers for pointer casts.
lib/CodeGen/CGExpr.cpp | ||
---|---|---|
3530 ↗ | (On Diff #96312) | Cool, thanks for the tip |
Looks great, thanks. One request for additional tests, but feel free to commit when that's ready.
test/CodeGenCXX/strict-vtable-pointers.cpp | ||
---|---|---|
258 ↗ | (On Diff #100533) | Please include test cases involving classes that only have a virtual pointer in a field of a base class — e.g. something that inherits from HoldingVirtuals. And be sure to cover both normal and virtual inheritance. |
test/CodeGenCXX/strict-vtable-pointers.cpp | ||
---|---|---|
263 ↗ | (On Diff #100621) | "virtual" applies to an individual base, not to all following bases. You need "virtual HoldingVirtuals" here. Although, come to think of it, having a virtual base should already ensure that the class has a v-table, even if it has no other reason for one. (You should add a test for that!) |