[clang] Fix crash for sizeof on VLAs

[pmatos]

Summary

Adds overload of TransformToPotentiallyEvaluated for TypeSourceInfo to
properly deal with VLAs in nested calls of sizeof and typeof. Fixes
PR31042 (https://github.com/llvm/llvm-project/issues/30390).

[pmatos]

@efriedma Here's the new patch, thanks for your help getting here.

[ahatanak]

Can you add a test case?

[efriedma]

Missing regression test in test/SemaCXX/.

[pmatos]

Apologies for the delay over the holiday season, I will look into this later on after office hours.

[pmatos]

Has this been fixed upstream already?

[pmatos]

Ah no, my mistake. I had the patch applied when I ran the test.

[pmatos]

Here's an updated patch including the test. Hope this is now ok for submission. Please accept my apologies with regards to the delay in submitting this.

[efriedma]

lib/Sema/SemaExpr.cpp

4031 ↗

(On Diff #85328)

Is the isUnevaluatedContext() check here necessary?

test/SemaCXX/pr31042.cpp

1 ↗

(On Diff #85328)

-emit-obj ?

3 ↗

(On Diff #85328)

Is this declaration necessary somehow?

[pmatos]

Thanks for the comments.

[pmatos]

lib/Sema/SemaExpr.cpp

4031 ↗

(On Diff #85328)

Not really. Reran tests to ensure removing it doesn't break anything.

test/SemaCXX/pr31042.cpp

1 ↗

(On Diff #85328)

Not needed. Left over from copying another test.

3 ↗

(On Diff #85328)

Same. Not needed. Left over from copying another test.

[pmatos]

@efriedma I have uploaded a new patch taking your comments into consideration and rebased on most recent clang sources.

[pmatos]

Added missing testcase to previous patch.

[efriedma]

test/SemaCXX/pr31042.cpp

1 ↗

(On Diff #91685)

Oh, this testcase doesn't actually crash on trunk without at least -emit-llvm because semantic analysis doesn't actually verify the used bit. :( Better to include that, I think.

[efriedma]

test/SemaCXX/pr31042.cpp

1 ↗

(On Diff #91980)

You need to use "-o -" or something like that to avoid generating a file pr31042.ll. Also, a comment explaining why this test is using -emit-llvm would be nice.

[pmatos]

After a long hiatus on this bug, this is still failing on HEAD so lets get it fixed.

[pmatos]

This comment was removed by pmatos.

test/SemaCXX/pr31042.cpp

1 ↗

(On Diff #91685)

Ah, that's why I had initially -emit-obj. That also triggered the problem. I obviously forgot about this and removed the flag without retesting to check if still broke trunk. Apologies for that.

1 ↗

(On Diff #91685)

And we also need to remove -fsyntax-only. Submitting new patch.

[pmatos]

Fixes https://github.com/llvm/llvm-project/issues/30390

[pmatos]

@efriedma I know it has been a long time, but are you still able to review this?

[efriedma]

Looks like all the review comments have been addressed. LGTM

[pmatos]

In D27800#3235066, @efriedma wrote:

Looks like all the review comments have been addressed. LGTM

Thanks for the review, but unfortunately I found an issue right before committing, taking a look at it now.

[pmatos]

Ensure that we only call transform on Unevaluated Contexts, avoids the failure of a couple of vla tests.

[pmatos]

In D27800#3235066, @efriedma wrote:

Looks like all the review comments have been addressed. LGTM

Minor change.

clang/include/clang/Sema/Sema.h

void PushExpressionEvaluationContext(
       ExpressionEvaluationContext NewContext, ReuseLambdaContextDecl_t,
       ExpressionEvaluationContextRecord::ExpressionKind Type =
           ExpressionEvaluationContextRecord::EK_Other);
   void PopExpressionEvaluationContext();
 
   void DiscardCleanupsInEvaluationContext();
 
   ExprResult TransformToPotentiallyEvaluated(Expr *E);
+  TypeSourceInfo *TransformToPotentiallyEvaluated(TypeSourceInfo *TInfo);
   ExprResult HandleExprEvaluationContextForTypeof(Expr *E);
 
   ExprResult CheckUnevaluatedOperand(Expr *E);
   void CheckUnusedVolatileAssignment(Expr *E);
 
   ExprResult ActOnConstantExpression(ExprResult Res);
 
   // Functions for marking a declaration referenced.  These functions also

clang/lib/Sema/SemaExpr.cpp

if (auto *TT = T->getAs<TypedefType>()) {
             break;
           captureVariablyModifiedType(Context, T, CSI);
         }
       }
     }
   }
 
   // C99 6.5.3.4p4: the type (an unsigned integer type) is size_t.
+  if (isUnevaluatedContext() && ExprKind == UETT_SizeOf &&
+      TInfo->getType()->isVariablyModifiedType())
+    TInfo = TransformToPotentiallyEvaluated(TInfo);
+
   return new (Context) UnaryExprOrTypeTraitExpr(
       ExprKind, TInfo, Context.getSizeType(), OpLoc, R.getEnd());
 }
 
 /// Build a sizeof or alignof expression given an expression
 /// operand.
 ExprResult
 Sema::CreateUnaryExprOrTypeTraitExpr(Expr *E, SourceLocation OpLoc,
assert(isUnevaluatedContext() &&
          "Should only transform unevaluated expressions");
   ExprEvalContexts.back().Context =
       ExprEvalContexts[ExprEvalContexts.size()-2].Context;
   if (isUnevaluatedContext())
     return E;
   return TransformToPE(*this).TransformExpr(E);
 }
 
+TypeSourceInfo *Sema::TransformToPotentiallyEvaluated(TypeSourceInfo *TInfo) {
+  assert(isUnevaluatedContext() &&
+         "Should only transform unevaluated expressions");
+  ExprEvalContexts.back().Context =
+      ExprEvalContexts[ExprEvalContexts.size() - 2].Context;
+  if (isUnevaluatedContext())
+    return TInfo;
+  return TransformToPE(*this).TransformType(TInfo);
+}
+
 void
 Sema::PushExpressionEvaluationContext(
     ExpressionEvaluationContext NewContext, Decl *LambdaContextDecl,
     ExpressionEvaluationContextRecord::ExpressionKind ExprContext) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
                                 LambdaContextDecl, ExprContext);
 
   // Discarded statements and immediate contexts nested in other

clang/test/SemaCXX/pr31042.cpp

+// RUN: %clang_cc1 -o - -emit-llvm -triple x86_64-unknown-linux-gnu -disable-free %s
+// We need to use -emit-llvm in order to trigger the error, without it semantic analysis
+// does not verify the used bit and there's no error.
+
+char a[1];
+
+void f1(void) {
+  int i = 0;
+  int j = sizeof(typeof(*(char(*)[i])a));
+}