This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
ClosedPublic

Authored by Prazek on May 23 2018, 4:13 PM.

Diff Detail

Event Timeline

Prazek created this revision.May 23 2018, 4:13 PM
rsmith added inline comments.May 23 2018, 5:44 PM
clang/include/clang/AST/DeclCXX.h
779

isCompleteDefinition checks whether this declaration of the class is a definition, not whether it has a definition anywhere; the latter is what you need here. You can use hasDefinition to check that.

Please also check hasAnyDependentBases() (or add a comment to this function to indicate that it may return false for a templated class whose instantiations might be dynamic classes) -- if a class has dependent bases, we might not find out that it's a dynamic class until it's instantiated.

clang/lib/CodeGen/CGExprScalar.cpp
1626–1627

Are there any cases where we need a barrier when the destination type is a dynamic type here?

Prazek added inline comments.May 25 2018, 5:44 PM
clang/lib/CodeGen/CGExprScalar.cpp
1626–1627

No, I don't think so. I will also add test for that.

Prazek updated this revision to Diff 148745.May 27 2018, 5:07 AM
Added launder when going from possiblyNotDynamic to possiblyDynamic
emitting strip for pointer -> int only if poitner is possiblyDynamic
Prazek updated this revision to Diff 148746.May 27 2018, 5:11 AM
Prazek marked 3 inline comments as done.

one more test

rjmccall added inline comments.Jun 11 2018, 11:32 PM
clang/include/clang/AST/DeclCXX.h
784

Both of these new methods deserve doc comments explaining that they're conservative checks because the class might be incomplete or dependent.

I think NonDynamic would read better than NotDynamic.

clang/lib/CodeGen/CGExprScalar.cpp
1623

Unnecessary getTypePtr().

1624

The opposite of Dst is Src. Alternatively, the opposite of Source is Destination (or Result). Please pick.

1633

If you made a couple of tiny helper functions here that you could invoke on either SourceClassDecl or DstClassDecl, you could avoid some redundant logic and also make the calls self-documenting enough to legibly inline into the if-conditions.

...in fact, since you start from a QualType in every case, maybe you should just define the helper as a method there.

1647

Incidentally, how do you protect against code like this?

A *ptr;
reinterpret_cast<B *&>(ptr) = new B();
ptr->foo();

Presumably there needs to be a launder/strip here, but I guess it would have to be introduced by the middle-end when forwarding the store? The way I've written this is an aliasing violation, but (1) I assume your pass isn't disabled whenever strict-aliasing is disabled and (2) you can do this with a memcpy and still pretty reliably expect that LLVM will be able to eventually forward the store.

1791

Another place you could definitely just use that helper function on QualType.

1802

Same.

3305

Yeah, helper function on QualType, please. :)

Prazek marked 5 inline comments as done.Jun 15 2018, 3:31 PM
Prazek added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
1623

getType returns QualType and it does not have getPointeeCXXRecordDecl. Am I missing something?

1647

Can you add more info on what is A and B so I can make sure I understand it correctly?
Is the prefix of the layout the same for both, but they are not in the hierarchy?

I haven't thought about the strict aliasing. I think the only sane way would be to require strict aliasing for the strict vtable pointers.

Prazek updated this revision to Diff 151575.Jun 15 2018, 3:36 PM

Refactor

clang/lib/CodeGen/CGExprScalar.cpp
1633

oh yea, it is definitely better!

Prazek marked an inline comment as done.Jun 15 2018, 3:38 PM
Prazek added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
1624

Not sure if it still holds (if the DestTy should be DstTy, but I haven't declared it myself)

rjmccall added inline comments.Jun 15 2018, 3:41 PM
clang/lib/CodeGen/CGExprScalar.cpp
1623

operator-> on QualType drills to the Type*.

1647

It's whatever case you're worried about such that you have to launder member accesses and bitcasts.

And like I mentioned, relying on strict aliasing isn't enough because you can do it legally with memcpy. Maybe it's okay to consider it UB? I'm not sure about that.

Prazek added inline comments.Jun 16 2018, 10:56 AM
clang/lib/CodeGen/CGExprScalar.cpp
1647

AFAIK reinterpreting one class as another is UB if they are not in hierarchy (especially calling virtual function on reinterpreted class), not sure if strict aliasing should allow it anyway (if it would be a hand written custom vptr then it should be ok with strict aliasing turned off, but with vptr I don't think it is legal).
@rsmith Can you comment on that?

rsmith added inline comments.Jun 28 2018, 11:19 AM
clang/lib/CodeGen/CGExprScalar.cpp
1647

OK, here's how I think about what we're doing here:

We view the IR-level pointer value for a pointer to a dynamic class type as being a fat pointer, containing the actual pointer value and also a tag indicating the dynamic type of the object (only notionally, though -- the actual bit representation of the pointer doesn't include the extra information, but we don't ever emit IR that inspects the bit representation of the fat pointer to avoid exposing that fact). In that model, if you try to type pun between a pointer to a dynamic class type and a pointer to a non-dynamic-class type, that can't be expected to work because the (notional) value is different, much as type punning between a derived and base class pointer wouldn't work for a pointer to something other than a base class at offset zero.

I think @rjmccall's example is OK, because both A and B would need to be dynamic class types in a hierarchy to work, and that means we'd be using the same notional pointer representation. A slight variation of that example:

struct A {};
struct B : A { virtual void f(); };
struct C : B { void f(); } c;
A *p = &c;
B *q;
memcpy(&q, &p, sizeof(B*)); // or q = std::bit_cast<B*>(p);
q->f();

... would be UB, because the representation of an A* and a B* are different (a B* contains a tag and an A* does not).

Prazek marked 10 inline comments as done.Jul 1 2018, 10:35 PM
Prazek added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
1647

Does this answer satisfy you John? Can I push it to trunk?

rjmccall added inline comments.Jul 2 2018, 11:50 AM
clang/lib/CodeGen/CGExprScalar.cpp
1647

Yeah, Richard's answer makes sense to me.

Prazek marked 3 inline comments as done.Jul 2 2018, 12:25 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 2 2018, 12:26 PM
This revision was automatically updated to reflect the committed changes.