[CGExprAgg] Fix infinite loop in `findPeephole`

[ekatz]

Summary

Simplify the function using IgnoreParenNoopCasts.

Fix PR45476

[lebedev.ri]

Can test be added here?

[ekatz]

Added test.

[rjmccall]

clang/lib/CodeGen/CGExprAgg.cpp

687

I liked the structure of the old code better, in case we want to look through other kinds of expressions. Please just add op = castE->getSubExpr() before the continue.

[ekatz]

clang/lib/CodeGen/CGExprAgg.cpp

687

I see your point. I'll change that.
Though I must say that the old structure is a little strange with the return nullptr in the end of the loop...

[ekatz]

Use the old structure of the algorithm, with the fix, as requested.

[rjmccall]

clang/lib/CodeGen/CGExprAgg.cpp

687

Oh, you know, there's also an IgnoreParenNoopCasts that we could just use instead of this loop if we're willing to ignore other possible expressions we might want to look through.

[ekatz]

clang/lib/CodeGen/CGExprAgg.cpp

687

I am not familiar enough with clang's code to make the decision to remove this function.
Personally I wanted to fix the bug, and I guess this function is here for a reason..?

[rjmccall]

clang/lib/CodeGen/CGExprAgg.cpp

687

I don't think there's anything here really worth keeping. Please keep the function around but replace its body with something like:

op = op->IgnoreParenNoopCasts();
if (auto castE = dyn_cast<CastExpr>(op)) {
  if (castE->getCastKind() == kind)
    return castE->getSubExpr();
}
return nullptr;

[ekatz]

clang/lib/CodeGen/CGExprAgg.cpp

687

Sorry, I, kinda, misunderstood you suggestion before.
This is a really good implementation, but as you said, this function should, and I quote from its description, "look through various unimportant expressions". So I guess it is better to leave it this way (with the loop), as it is already in the base code. What do you say?

[ekatz]

clang/lib/CodeGen/CGExprAgg.cpp

687

I'll counter myself. (COVID-19 + Kids = TOTAL Tiredness)
This still gives us the opportunity to add "various unimportant expressions". I'll change to the suggested implementation.
Thanks! :)

[ekatz]

Simplify function using IgnoreParenNoopCasts.

[rjmccall]

Thanks, LGTM.

[rjmccall]

Oh, no, not quite. Please name the test case to something more specific; pr45476.cpp would be fine.

[ekatz]

Changed the name of the test file to pr45476.cpp.

clang/lib/CodeGen/CGExprAgg.cpp

if (Destruct)
     if (QualType::DestructionKind DtorKind = E->getType().isDestructedType())
       CGF.pushLifetimeExtendedDestroy(
           CGF.getCleanupKind(DtorKind), Slot.getAddress(), E->getType(),
           CGF.getDestroyer(DtorKind), DtorKind & EHCleanup);
 }
 
 /// Attempt to look through various unimportant expressions to find a
 /// cast of the given kind.
-static Expr *findPeephole(Expr *op, CastKind kind) {
-  while (true) {
-    op = op->IgnoreParens();
-    if (CastExpr *castE = dyn_cast<CastExpr>(op)) {
-      if (castE->getCastKind() == kind)
-        return castE->getSubExpr();
-      if (castE->getCastKind() == CK_NoOp)
-        continue;
-    }
-    return nullptr;
-  }
-}
+static Expr *findPeephole(Expr *op, CastKind kind, const ASTContext &ctx) {
+  op = op->IgnoreParenNoopCasts(ctx);
+  if (auto castE = dyn_cast<CastExpr>(op)) {
 
 void AggExprEmitter::VisitCastExpr(CastExpr *E) {
   if (const auto *ECE = dyn_cast<ExplicitCastExpr>(E))
     CGF.CGM.EmitExplicitCastExprType(ECE, &CGF);
   switch (E->getCastKind()) {
   case CK_Dynamic: {
     // FIXME: Can this actually happen? We have no test coverage for it.
     assert(isa<CXXDynamicCastExpr>(E) && "CK_Dynamic without a dynamic_cast?");
case CK_AtomicToNonAtomic: {
     if (Dest.isIgnored() || !CGF.CGM.isPaddedAtomicType(atomicType)) {
       return Visit(E->getSubExpr());
     }
 
     CastKind peepholeTarget =
       (isToAtomic ? CK_AtomicToNonAtomic : CK_NonAtomicToAtomic);
 
     // These two cases are reverses of each other; try to peephole them.
-    if (Expr *op = findPeephole(E->getSubExpr(), peepholeTarget)) {
+    if (Expr *op =
+            findPeephole(E->getSubExpr(), peepholeTarget, CGF.getContext())) {
       assert(CGF.getContext().hasSameUnqualifiedType(op->getType(),
                                                      E->getType()) &&
            "peephole significantly changed types?");
       return Visit(op);
     }
 
     // If we're converting an r-value of non-atomic type to an r-value
     // of atomic type, just emit directly into the relevant sub-object.

clang/test/CodeGen/pr45476.cpp

+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+// PR45476
+
+// This test used to get into an infinite loop,
+// which, in turn, caused clang to never finish execution.
+
+struct s3 {
+  char a, b, c;
+};
+
+_Atomic struct s3 a;
+
+extern "C" void foo() {
+  // CHECK-LABEL: @foo
+  // CHECK: store atomic i32
+
+  a = s3{1, 2, 3};
+}
+