[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

688

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

688

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

688

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

688

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

688

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

688

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

688

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 (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;
-  }
-}
+  while (CastExpr *castE = dyn_cast<CastExpr>(op->IgnoreParens())) {
+
+    if (castE->getCastKind() != CK_NoOp || castE == op)
+      break;
+
+    op = castE;
 
 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?");