[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:784
+    return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases();
+  }
+
----------------
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`.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623
+      const CXXRecordDecl *SourceClassDecl =
+          E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+      const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
----------------
Unnecessary `getTypePtr()`.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1624
+          E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+      const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
+
----------------
The opposite of `Dst` is `Src`.  Alternatively, the opposite of `Source` is `Destination` (or `Result`).  Please pick.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1633
+      bool DstMayNotBeDynamic =
+          !DstClassDecl || DstClassDecl->mayBeNotDynamicClass();
+      if (SourceMayBeNotDynamic && DstMayBeDynamic) {
----------------
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.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp: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.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1791
+      // dynamic information from invariant.group.
+      if (DstClassDecl && DstClassDecl->mayBeDynamicClass())
+        IntToPtr = Builder.CreateLaunderInvariantGroup(IntToPtr);
----------------
Another place you could definitely just use that helper function on QualType.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1802
+      const CXXRecordDecl *ClassDecl =
+          E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+      // Casting to integer requires stripping dynamic information as it does
----------------
Same.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3305
+          if (RD->mayBeDynamicClass())
+            RHS = Builder.CreateStripInvariantGroup(RHS);
+      }
----------------
Yeah, helper function on QualType, please. :)


Repository:
  rL LLVM

https://reviews.llvm.org/D47299