This is an archive of the discontinued LLVM Phabricator instance.

Compound literals, enums, et al require const expr
ClosedPublic

Authored by void on Oct 31 2018, 1:27 AM.

Details

Summary

Compound literals, enums, file-scoped arrays, etc. require their
initializers and size specifiers to be constant. Wrap the initializer
expressions in a ConstantExpr so that we can easily check for this later
on.

Diff Detail

Repository
rL LLVM

Event Timeline

void created this revision.Oct 31 2018, 1:27 AM
lib/Sema/SemaDecl.cpp
16411 ↗(On Diff #171868)

probably don't need to adjust this line

void updated this revision to Diff 171976.Oct 31 2018, 11:29 AM
void retitled this revision from Compound literals and enums require constant inits to Compound literals, global array decls, and enums require constant inits.
void edited the summary of this revision. (Show Details)
void added inline comments.
lib/Sema/SemaDecl.cpp
16411 ↗(On Diff #171868)

I kinda felt compelled to do it when I saw it. Became twitchy. :-)

rsmith added inline comments.Oct 31 2018, 1:55 PM
lib/AST/ExprConstant.cpp
5266–5268 ↗(On Diff #171976)

This shouldn't be necessary because you changed the CRTP base class ExprEvaluatorBase to do this already.

5744–5746 ↗(On Diff #171976)

Likewise here.

lib/Sema/SemaDecl.cpp
16086–16094 ↗(On Diff #171976)

I think it would make sense to create the ConstantExpr wrapper node in CheckConvertedConstantExpr / VerifyIntegerConstantExpr. (That's also where it'd make sense to cache the evaluated value on the wrapper node once we start doing so.)

lib/Sema/SemaExpr.cpp
5752 ↗(On Diff #171976)

Can we create the ConstantExpr in here instead (assuming we need it at all)?

(The reason it's not clear to me that we need it at all is that file-scope compound literals can only appear in contexts where a constant expression is required *anyway* -- be they in array bounds, enumerators, or the initializer of a global variable.)

5789 ↗(On Diff #171976)

(This approach is at least not entirely correct: compound literals are only required to have a constant initializer at file scope.)

lib/Sema/SemaType.cpp
2181 ↗(On Diff #171976)

This call calls VerifyIntegerConstantExpression; I think we should be creating the ConstantExpr node within there when appropriate.

void marked 3 inline comments as done.Oct 31 2018, 8:53 PM
void added inline comments.
lib/Sema/SemaDecl.cpp
16086–16094 ↗(On Diff #171976)

I thought the purpose of ConstantExpr is to specify those places where a constant expression is required. I.e., we can't have something like:

int z;
foo y = (foo){z + 2};

In this case, z + 2 would be wrapped by the ConstantExpr class. But in a function or module scope, then it would be fine:

void x(int z) {
  foo y = (foo){z + 2};
}

So z + 2 wouldn't be wrapped. If I perform the wrapping in CheckConvertedConstantExpr, et al, then it doesn't seem like I have the context to say that it's a) a compound literal, and b) in file scope. So how can I correctly wrap it?

lib/Sema/SemaExpr.cpp
5752 ↗(On Diff #171976)

I changed the code to wrap around here. But see my comment above. I think we might be thinking about ConstantExpr differently.

void updated this revision to Diff 172082.Oct 31 2018, 8:54 PM
void updated this revision to Diff 172194.Nov 1 2018, 11:48 AM
void updated this revision to Diff 172279.Nov 1 2018, 5:14 PM
void retitled this revision from Compound literals, global array decls, and enums require constant inits to Compound literals, enums, et al require const expr.
void edited the summary of this revision. (Show Details)

I believe this patch should address most, if not all, of your concerns. PTAL.

void updated this revision to Diff 172297.Nov 1 2018, 7:43 PM
void updated this revision to Diff 172313.Nov 2 2018, 12:11 AM
rsmith accepted this revision.Nov 8 2018, 2:44 PM

Thanks, other than the compound literal part I think this is all fine. I'm happy for you to go ahead with this as-is; we can rearrange how we handle compound literals as a later change if you prefer.

include/clang/AST/Expr.h
3073–3074 ↗(On Diff #172313)

Should we skip an arbitrary FullExpr here (and in the other Ignore functions below)?

lib/Sema/SemaDecl.cpp
16086–16094 ↗(On Diff #171976)

In the above example, I think we should have a ConstantExpr wrapping the entire initializer of y, because the whole thing is required to be a constant expression. Eg,

int z = 123;

... would *also* have a ConstantExpr wrapped around it -- indeed, in C, there should be a ConstantExpr around the initializer of all global variables, because they're all required to be initialized by constant expressions. So that's why I think the compound-literal part is a red herring.

lib/Sema/SemaType.cpp
2236–2238 ↗(On Diff #172313)

As noted above, I'd prefer for VerifyIntegerConstantExpression to create this. But if we need to do it here, we should do it on the code path that creates a ConstantArrayType, not based on whether the array type appears at file scope.

lib/StaticAnalyzer/Core/ExprEngine.cpp
1286–1288 ↗(On Diff #172313)

Maybe just share the code for the two FullExpr cases?

This revision is now accepted and ready to land.Nov 8 2018, 2:44 PM
void marked 2 inline comments as done.Nov 8 2018, 3:37 PM
void added inline comments.
include/clang/AST/Expr.h
3073–3074 ↗(On Diff #172313)

Sure. That seems reasonable.

lib/Sema/SemaType.cpp
2236–2238 ↗(On Diff #172313)

I think my next patch takes care of this (moves it to VerifyIntegerConstantExpression). There may be conflicts between the two patches though, so I'd like to move this there in the follow-up patch.

void marked an inline comment as done.Nov 8 2018, 4:26 PM

I'm using "arcanist" for the first time, and I don't want to mess things up. I'll address the comments here in the next patch.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.