[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

4027

Is the isUnevaluatedContext() check here necessary?

test/SemaCXX/pr31042.cpp

2

-emit-obj ?

4

Is this declaration necessary somehow?

[pmatos]

Thanks for the comments.

[pmatos]

lib/Sema/SemaExpr.cpp

4027

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

test/SemaCXX/pr31042.cpp

2

Not needed. Left over from copying another test.

4

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

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

2

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

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

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.

include/clang/Sema/Sema.h

public:
   void PushExpressionEvaluationContext(ExpressionEvaluationContext NewContext,
                                        ReuseLambdaContextDecl_t,
                                        bool IsDecltype = false);
   void PopExpressionEvaluationContext();
 
   void DiscardCleanupsInEvaluationContext();
 
   ExprResult TransformToPotentiallyEvaluated(Expr *E);
+  TypeSourceInfo *TransformToPotentiallyEvaluated(TypeSourceInfo *TInfo);
   ExprResult HandleExprEvaluationContextForTypeof(Expr *E);
 
   ExprResult ActOnConstantExpression(ExprResult Res);
 
   // Functions for marking a declaration referenced.  These functions also
   // contain the relevant logic for marking if a reference to a function or
   // variable is an odr-use (in the C++11 sense).  There are separate variants
   // for expressions referring to a decl; these exist because odr-use marking

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 (ExprKind == UETT_SizeOf &&
+      TInfo->getType()->isVariablyModifiedType())
+    TInfo = TransformToPotentiallyEvaluated(TInfo);
+  
   return new (Context) UnaryExprOrTypeTraitExpr(
       ExprKind, TInfo, Context.getSizeType(), OpLoc, R.getEnd());
 }
 
 /// \brief 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,
                                       bool IsDecltype) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
                                 LambdaContextDecl, IsDecltype);
   Cleanup.reset();
   if (!MaybeODRUseExprs.empty())

test/SemaCXX/pr31042.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -disable-free %s
+
+char a[1];
+
+void
+f1 (void)
+{
+  int i = 0;
+  int j = sizeof (typeof (*(char (*)[i])a));
+}