Page MenuHomePhabricator

PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.
ClosedPublic

Authored by rsmith on Feb 8 2019, 4:05 PM.

Details

Summary

We used to get this wrong in three ways:

  1. During parsing, an expression-statement followed by the }) ending a statement expression was always treated as producing the value of the statement expression. That's wrong for ({ if (1) expr; })
  2. During template instantiation, various kinds of statement (most statements not appearing directly in a compound-statement) were not treated as discarded-value expressions, resulting in missing volatile loads (etc).
  3. In all contexts, an expression-statement with attributes was not treated as producing the value of the statement expression, eg ({ [[attr]] expr; }).

Diff Detail

Repository
rC Clang

Event Timeline

rsmith created this revision.Feb 8 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 4:05 PM
rsmith retitled this revision from PR40642: Fix determination of whether the final statement of a statement expression is a discarded-value expression. to PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression..Feb 8 2019, 4:30 PM
rsmith edited the summary of this revision. (Show Details)

Thank you for working on this! It looks like we did have to add the new AST node after all, but the changes seem pretty good. Should anything be updated in the AST dumper for this (such as printing whether a Stmt is a value-producing statement or not)?

include/clang/AST/Stmt.h
1598–1602

We typically implement this in reverse, where the non-const version holds the actual implementation and the const version performs a const_cast.

include/clang/Parse/Parser.h
374

Rather than passing around IsInStmtExpr to a bunch of parser APIs, would it make more sense to add an RAII object that sets some state on Parser to test it? The proliferation of arguments seems unfortunate given how much of a corner case statement expressions are.

lib/Sema/SemaExpr.cpp
13337–13340

const auto *

13373

auto *

rsmith updated this revision to Diff 186337.Feb 11 2019, 2:26 PM
rsmith marked an inline comment as done.

Address a couple of review comments.

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

include/clang/AST/Stmt.h
1598–1602

We do. Do you think that's preferable? I like that this way around proves that the const version is const-correct, but it's a tiny benefit and I'm fine with just being consistent.

include/clang/Parse/Parser.h
374

Yeah, I tried that approach first, but the parser state turns out to be much worse, because it puts a burden on every form of statement that constructs a nested parsing context to reset that state. We can put the resetting on ParseScope, but it still needs to have an effect in the case where the scope is disabled, which is surprising, and there's no guarantee such statement constructs that can end in an expression will have a nested scope (consider, for instance, constructs like return x; or goto *label;). This is really local state that should only be propagated through a very small number of syntactic constructs rather than global state.

Maybe we could combine the new flag with the AllowOpenMPStandalone / AllowedConstructsKind flag into a more general kind of "statement context". The propagation rules aren't quite the same (AllowOpenMPStandalone doesn't get propagated through labels whereas IsInStmtExpr does), which is a little awkward, but maybe that's not so bad -- and maybe that's actually a bug in the OpenMP implementation?

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

Great, then I'll unify this new flag with the Allowed mechanism and fix the bug as part of this change. Thanks!

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

Great, then I'll unify this new flag with the Allowed mechanism and fix the bug as part of this change. Thanks!

Sure, thanks!

Considering that this has been fertile ground for buggy edge cases, should we revert my original changes from the 8.0 branch to give this more time to bake on trunk before releasing? Or do you think this is fine to move onto the release branch once finalized?

include/clang/AST/Stmt.h
1598–1602

Personally, I prefer the way you have it here (though I wish we had a global helper function to hide the dispatch dance).

include/clang/Parse/Parser.h
374

Maybe we could combine the new flag with the AllowOpenMPStandalone / AllowedConstructsKind flag into a more general kind of "statement context".

That seems like a sensible approach to me.

rsmith updated this revision to Diff 186578.Feb 12 2019, 6:11 PM

Combine WithinStmtExpr flag with AllowedStmtKinds into a more general statement
context. In doing so, fix some bugs where the OpenMP context was not being
propagated properly through labels and expression-statements starting with @.

aaron.ballman added inline comments.Feb 13 2019, 7:35 AM
include/clang/Parse/Parser.h
374

It's a bit strange that the previous two enumerators are "Allow" and this is "In". Maybe it will be less of a concern when I see the uses though...

398

Spurious newline.

lib/Parse/ParseStmt.cpp
1038
ParsedStmtContext SubStmtCtx =
      ParsedStmtContext::Compound |
      (isStmtExpr ? ParsedStmtContext::InStmtExpr : 0);

?

1054

You can drop the nullptr here.

1091–1092

Should this be done as part of handleExprStmt()?

ABataev added inline comments.Feb 13 2019, 9:34 AM
include/clang/Parse/Parser.h
382

We have llvm/ADT/BitmaskEnum.h, which defines LLVM_MARK_AS_BITMASK_ENUM for the bitwise ops on the enums. Maybe it is better to use it rather than manually define the required operations?

lib/CodeGen/CGStmt.cpp
401

You don't need else if here, just ]ша] is enough since there is break; in the previous substatement.

rsmith updated this revision to Diff 186731.Feb 13 2019, 1:38 PM
rsmith marked 7 inline comments as done.

Address review comments.

include/clang/Parse/Parser.h
374

It's just a coincidence that the first two are both Allow* flags.

382

That makes usage of the context a bit more verbose (StmtCtx & X -> (StmtCtx & X) != ParsedStmtContext()), but I think that's acceptable.

lib/Parse/ParseStmt.cpp
1038

There's no implicit conversion from 0 to ParsedStmtContext, but yeah, something like that :)

1091–1092

In principle that might make sense, but it'd require passing the attributes through a few extra layers, and also making sure the attributes aren't handled twice along the more-common statement parsing codepaths. I'm inclined to defer that until we have a need to have the attributes in hand when processing a full-expression (and I suspect that time may never come).

aaron.ballman accepted this revision.Feb 14 2019, 11:30 AM

LGTM!

lib/Parse/ParseStmt.cpp
1091–1092

Yeah, I think it's worth delaying that change then. Thank you for the explanation!

This revision is now accepted and ready to land.Feb 14 2019, 11:30 AM
This revision was automatically updated to reflect the committed changes.