[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
-                            /*Delegating=*/false, addr);
+                            /*Delegating=*/false, addr, type);
 }
----------------
mantognini wrote:
> rjmccall wrote:
> > mantognini wrote:
> > > This is the only place where the new parameter has an interesting value. In the ~10 other calls to `EmitCXXDestructorCall` overloads, the default value of this new parameter is used instead.
> > Arguments that are potentially required for correctness — as opposed to just enabling some optimization — should generally not have defaults.  I think you should remove these defaults and require a sensible type to be passed down in all cases.
> I've addressed that. I had to add some extra parameter/attributes here and there. Please let me know if anything is can be improved.
Thanks, other than the FIXMEs and the object/pointer mismatches this all looks right; very nicely done.


================
Comment at: clang/lib/CodeGen/CGClass.cpp:496
+      // destroyed should have the expected type.
+      QualType ThisTy = D->getThisType();
       Address Addr =
----------------
I think the rule we want is that the type passed here is the (qualified) object type, but `getThisType()` will return a pointer type.  Consider adding a `getThisObjectType()` method to `CXXMethodDecl` which does that computation (and then make `getThisType()` just wrap that in a `PointerType`).


================
Comment at: clang/lib/CodeGen/CGClass.cpp:1447
+    if (HaveInsertPoint()) {
+      // FIXME: Determine the type of the object being destroyed.
+      QualType ThisTy;
----------------
mantognini wrote:
> I'm not familiar enough with CXXCtorType and such, and would welcome some help for these two FIXMEs.
`Dtor->getThisObjectType()` should be right in both of these cases.


================
Comment at: clang/lib/CodeGen/CGClass.cpp:2376
+      // Therefore, "this" should have the expected type.
+      QualType ThisTy = Dtor->getThisType();
       CGF.EmitCXXDestructorCall(Dtor, Type, /*ForVirtualBase=*/false,
----------------
Same thing about `getThisObjectType()`.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+  // we ensure a cast is added where necessary.
+  if (ThisTy.isNull()) {
+#ifndef NDEBUG
----------------
mantognini wrote:
> Despite no longer having a default parameter, not all call site can provide a meaningful value ATM. That is why this check is still required.
Is that resolved with fixing the FIXME?

Please assert that `ThisTy->getAsCXXRecordDecl() == Dtor->getParent()` to guard against that pointer/object mixup.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:374
       EmitCXXDestructorCall(GD, Callee, This.getPointer(),
+                            /*ThisTy=*/QualType(),
                             /*ImplicitParam=*/nullptr,
----------------
`ThisTy` here can be either `Base->getType()` or `Base->getType()->getPointeeType()` depending on whether this is an arrow access.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1758
+  // This is suboptimal for correctness purposes. Instead, CE should probably
+  // always be defined.
+  QualType ThisTy;
----------------
It looks like the only places that pass down a null CE are the two implementations in `emitVirtualObjectDelete`, which both have a `CXXDeleteExpr`.  You could just have this method take an `llvm::PointerUnion<CXXMethodCallExpr*,CXXDeleteExpr*>` or something, and then someone else could take advantage of that for better source locations on the virtual call eventually.


================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1928
+  if (CE)
+    ThisTy = CE->getImplicitObjectArgument()->getType();
+
----------------
Same point as in the ItaniumCXXABI implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64569/new/

https://reviews.llvm.org/D64569