[Clang] Switch from TransformExpr to TransformInitializer in places we need to revert initializer to it syntactic form for Sema

[shafik]

Summary

In some cases we are using TransformExpr instead of TransformInitializer, this results in ExprWithCleanups being dropped and we are not emitting a destructor as a result.

This fixes: https://github.com/llvm/llvm-project/issues/62818

[shafik]

clang/test/Analysis/missing-bind-temporary.cpp

10

@NoQ I had to update this test due to my fix, since this was documenting a flaw, I am not sure what you want to do with it.

I think it is still useful as a regression and or verification test but you may want to modify it differently.

[erichkeane]

Change looks good, test is overly specific IMO.

clang/test/CodeGenCXX/gh62818.cpp

28

This test seems a little overly specific, right? I suspect you really only need call{{.*}} <ctor>, copy and dtor here. The rest seems unnecessary.

[shafik]

• Update test gh62818.cpp to be more precise

[Fznamznon]

clang/test/CodeGenCXX/gh62818.cpp

2

Why no opaque pointers? AFAIK since LLVM 17 typed pointers are not supported.

[shafik]

• Update test

[shafik]

clang/test/CodeGenCXX/gh62818.cpp

2

I had copied it from another test and missed that aspect. It seems like a bunch of tests in that dir use this argument.

clang/lib/Sema/TreeTransform.h

 
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformBinaryOperator(BinaryOperator *E) {
   ExprResult LHS = getDerived().TransformExpr(E->getLHS());
   if (LHS.isInvalid())
     return ExprError();
 
-  ExprResult RHS = getDerived().TransformExpr(E->getRHS());
+  ExprResult RHS =
+      getDerived().TransformInitializer(E->getRHS(), /*NotCopyInit=*/false);
   if (RHS.isInvalid())
     return ExprError();
 
   if (!getDerived().AlwaysRebuild() &&
       LHS.get() == E->getLHS() &&
       RHS.get() == E->getRHS())
     return E;
 
if (E->getOperator() == OO_Amp)
     First = getDerived().TransformAddressOfOperand(E->getArg(0));
   else
     First = getDerived().TransformExpr(E->getArg(0));
   if (First.isInvalid())
     return ExprError();
 
   ExprResult Second;
   if (E->getNumArgs() == 2) {
-    Second = getDerived().TransformExpr(E->getArg(1));
+    Second =
+        getDerived().TransformInitializer(E->getArg(1), /*NotCopyInit=*/false);
     if (Second.isInvalid())
       return ExprError();
   }
 
   if (!getDerived().AlwaysRebuild() &&
       Callee.get() == E->getCallee() &&
       First.get() == E->getArg(0) &&
       (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))

clang/test/Analysis/missing-bind-temporary.cpp

 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=debug.DumpCFG -std=c++14 -analyzer-config eagerly-assume=false %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=core,debug.ExprInspection -std=c++14 -verify -analyzer-config eagerly-assume=false %s
 
 void clang_analyzer_eval(bool);
 
 int global;
 
 namespace variant_0 {
-// This variant of the code works correctly. Function foo() is not a template
-// function. Note that there are two destructors within foo().
 
 class A {
 public:
   ~A() { ++global; }
 };
 
 class B {
   A a;
void bar() {
   global = 0;
   foo(1);
   clang_analyzer_eval(global == 2); // expected-warning{{TRUE}}
 }
 
 } // end namespace variant_0
 
 namespace variant_1 {
-// Suddenly, if we turn foo() into a template, we are missing a
-// CXXBindTemporaryExpr in the AST, and therefore we're missing a
-// temporary destructor in the CFG.
 
 class A {
 public:
   ~A() { ++global; }
 };
 
 class B {
   A a;
 };
 
-// FIXME: Find the construction context for {} and enforce the temporary
-// destructor.
 // CHECK: template<> void foo<int>(int)
 // CHECK:       [B1]
 // CHECK-NEXT:    1:  (CXXConstructExpr, [B1.2], B)
 // CHECK-NEXT:    2: B i;
 // CHECK-NEXT:    3: operator=
 // CHECK-NEXT:    4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, B &(*)(B &&) noexcept)
 // CHECK-NEXT:    5: i
-// CHECK-NEXT:    6: {} (CXXConstructExpr, B)
-// CHECK-NEXT:    7: [B1.6]
-// CHECK-NEXT:    8: [B1.5] = [B1.7] (OperatorCall)
-// CHECK-NEXT:    9: [B1.2].~B() (Implicit destructor)
+// CHECK-NEXT:    6: {} (CXXConstructExpr, [B1.7], [B1.8], B)
+// CHECK-NEXT:    7: [B1.6] (BindTemporary)
+// CHECK-NEXT:    8: [B1.7]
+// CHECK-NEXT:    9: [B1.5] = [B1.8] (OperatorCall)
+// CHECK-NEXT:    10: ~B() (Temporary object destructor)
+// CHECK-NEXT:    11: [B1.2].~B() (Implicit destructor)
 template <typename T> void foo(T) {
   B i;
   i = {};
 }
 
 void bar() {
   global = 0;
   foo(1);
-  // FIXME: Should be TRUE, i.e. we should call (and inline) two destructors.
-  clang_analyzer_eval(global == 2); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(global == 2); // expected-warning{{TRUE [debug.ExprInspection]}}
 }
 
 } // end namespace variant_1
 
 namespace variant_2 {
 // Making field 'a' in class 'B' public turns the class into an aggregate.
 // In this case there is no constructor at {} - only an aggregate
 // initialization. Aggregate initialization is unsupported for now.

clang/test/CodeGenCXX/gh62818.cpp

+// RUN: %clang_cc1 -std=c++17 -emit-llvm -triple x86_64-linux-gnu -o - %s | FileCheck %s
+
+void doSomething();
+
+struct A {
+  A() {};
+  ~A() noexcept {
+    doSomething();
+  }
+
+  A & operator=(A a) & noexcept {
+    return *this;
+  }
+};
+
+template<typename T>
+struct B {
+  void test() {a = {};}
+  // CHECK: define linkonce_odr void @_ZN1BIiE4testEv
+  // CHECK: call void @_ZN1AC1Ev(ptr noundef nonnull align 1 dereferenceable(1)
+  // CHECK: [[CALL:%.*]] = call noundef nonnull align 1 dereferenceable(1) ptr @_ZNR1AaSES_
+  // CHECK: call void @_ZN1AD2Ev(ptr noundef nonnull align 1 dereferenceable(1)
+
+  A a;
+};
+
+void client(B<int> &f) {f.test();}