Can't use sizeof ( typeof ( <vla> ))

Hello,

Clang seems to be happy with most simple vla but then it crashes with:

int
f1 (int i)
{
  char a[i];
  return sizeof (typeof (a));
}

The assertion failure is :
clang-3.8:
/home/pmatos/Clients/embecosm/llvm/tools/clang/lib/CodeGen/CGExpr.cpp:2113:
clang::CodeGen::LValue
clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(const
clang::DeclRefExpr*): Assertion `(ND->isUsed(false) || !isa<VarDecl>(ND)

!E->getLocation().isValid()) && "Should not use decl without marking

it used!"' failed.

Same issue with nightly.
I reported this as https://llvm.org/bugs/show_bug.cgi?id=31042

From looking at the Sema code it looks like it just assumes that

anything inside sizeof () is unevaluated:
  enum ExpressionEvaluationContext {
    /// \brief The current expression and its subexpressions occur within an
    /// unevaluated operand (C++11 [expr]p7), such as the subexpression of
    /// \c sizeof, where the type of the expression may be significant but
    /// no code will be generated to evaluate the value of the expression at
    /// run time.
    Unevaluated,

The problem with this rationale is that it's not true in this case since
you need to evaluate typeof (a), which can't be evaluated at compile
time. Is it true that to fix this issue, we need to rethink how sizeof
are evaluated?

Kind regards,

sizeof() and typeof() with an operand of VLA type are already special-cased; see Sema::TransformToPotentiallyEvaluated.

-Eli

Works fine with clang 3.8.1 here?

Joerg

The problem with this rationale is that it's not true in this case since
you need to evaluate typeof (a), which can't be evaluated at compile
time. Is it true that to fix this issue, we need to rethink how sizeof
are evaluated?

sizeof() and typeof() with an operand of VLA type are already
special-cased; see Sema::TransformToPotentiallyEvaluated.

In that case there's a bug in that special casing, since compiler is
crashing with the example I posted. I will try to find out why. Was not
aware of Sema::TransformToPotentiallyEvaluated.

I assume you didn't build with assertions on (debug build).

Yeah sorry, that was not my usual clang binary :slight_smile:

Joerg

Actually sizeof does not seem to be special cased. Only Typeof which
calls Sema::TransformToPotentiallyEvaluated through
Sema::HandleExprEvaluationContextForTypeof. Special-casing works by
making the typeof context evaluated if the previous context is evaluated:
ExprResult Sema::TransformToPotentiallyEvaluated(Expr *E) {
  assert(isUnevaluatedContext() &&
         "Should only transform unevaluated expressions");
  ExprEvalContexts.back().Context =
      ExprEvalContexts[ExprEvalContexts.size()-2].Context;
  if (isUnevaluatedContext())
    return E;
  return TransformToPE(*this).TransformExpr(E);
}

Unfortunately the previous context is a sizeof which is not evaluated
and therefore leaves the typeof as unevaluated, hence the bug.

git grep shows that TransformToPotentiallyEvaluated has three callers; one for typeof, one for sizeof, and one for typeid.

-Eli

That's correct but that's not the problem. It works ok for sizeof (vla)
and for typeof (vla) but not for sizeof (typeof (vla)). Note that
TransformToPotentiallyEvaluated is called for typeof (the inner
expression) and this function only actually does the transformation is
the sizeof (...) context is marked as PotentiallyEvaluated, which is not
in trunk.

I submitted a patch for review that fixes the issue (but causes others)
that mark the sizeof context as PotentiallyEvaluated, however this needs
changing.

If you have any suggestions on how to go about fixing this, I am all ears.

Kind regards,

The way it should work is that there are three calls to TransformToPotentiallyEvaluated: the first time, we're calling on the typeof, and it's unevaluated. The second time, we're calling it on the sizeof(), and it's potentially evaluated. The third time is a recursive call: in the process of transforming the sizeof(), we make a new typeof() expression, and call TransformToPotentiallyEvaluated on that; at that point, it should be potentially-evaluated. (Yes, this is terribly inefficient, but it's a rare situation anyway.)

-Eli

Apologies, only just managed to get back on this. I assume you're
suggesting a solution, not how it's implemented.

Currently there are two calls in SemaExpr.cpp and one in SemaExprCXX,cpp.

The two in SemaExpr.cpp are:

* in CreateUnaryExprOrTypeTraitExpr:

it happens when Expr is a Sizeof and argument is VLA.

* in HandleExprEvaluationContextforTypeof:

it checks if argument is vla and if so, transforms it to VLA if context
of typeof is PotentiallyEvaluated.

The one in SemaExprCXX.cpp is:

* in BuildCXXTypeId:

it is called when building a C++ Typeid.

Unfortunately for an expression in C like `sizeof(typeof(foo))` where
foo is a VLA, a context is created when parsing the sizeof which is
unevaluated and once typeof is parsed, it calls
HandleExprEvaluationContextforTypeof which since argument is vla, tries
to transform the context. However the context of typeof is unevaluated
so nothing changes.

Going back to what you said, there's currently no recursive call to
handle this. Was your message a suggestion for implementation?

I was going to implement it through extending the context types to add
UnevaluatedSizeof and detect this throughout the parser.

This is what's already implemented for sizeof() on a expression. We just don't do it properly for sizeof() on a type, I think. There are two functions named Sema::CreateUnaryExprOrTypeTraitExpr; compare the version which takes a type to the version which takes an expression.

-Eli

I see what you mean but it doesn't seem to be that straightforward. I
implemented the following patch which fixes the issue I was seeing but
crashes loads of other examples.

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index dbaa04e..ff04a64 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -3780,8 +3780,18 @@
Sema::CreateUnaryExprOrTypeTraitExpr(TypeSourceInfo *TInfo,
     return ExprError();

   // C99 6.5.3.4p4: the type (an unsigned integer type) is size_t.
- return new (Context) UnaryExprOrTypeTraitExpr(
+ Expr *E = new (Context) UnaryExprOrTypeTraitExpr(
       ExprKind, TInfo, Context.getSizeType(), OpLoc, R.getEnd());

I think you might need to add an overload of TransformToPotentiallyEvaluated: you should be transforming TInfo, not E. (Not 100% sure this matters in practice, but I think you could end up with infinite recursion.)

-Eli

Apologies if I misunderstand but you can't easily do that as you can't
do `TransformToPE(*this).TransformExpr(TInfo);`, you need to first
convert a TInfo into an Expr and then call it on
`Sema::TransformToPotentiallyEvaluated` so no overload seems to be
necessary.

You can do "TransformToPE(*this).TransformType(TInfo);"

-Eli