Page MenuHomePhabricator

[clang] C++98 implicit moves are back with a vengeance
ClosedPublic

Authored by mizvekov on Jul 9 2021, 7:18 PM.

Details

Summary

After taking C++98 implicit moves out in D104500,
we put it back in, but now in a new form which preserves
compatibility with pure C++98 programs, while at the same time
giving almost all the goodies from P1825.

  • We use the exact same rules as C++20 with regards to which id-expressions are move eligible. The previous incarnation would only benefit from the proper subset which is copy ellidable. This means we can implicit move, in addition:
    • Parameters.
    • RValue references.
    • Exception variables.
    • Variables with higher-than-natural required alignment.
    • Objects with different type from the function return type.
  • We preserve the two-overload resolution, with one small tweak to the first one: If we either pick a (possibly converting) constructor which does not take an rvalue reference, or a user conversion operator which is not ref-qualified, we abort into the second overload resolution.

This gives C++98 almost all the implicit move patterns which we had created test
cases for, while at the same time preserving the meaning of these
three patterns, which are found in pure C++98 programs:

  • Classes with both const and non-const copy constructors, but no move constructors, continue to have their non-const copy constructor selected.
  • We continue to reject as ambiguous the following pattern:
struct A { A(B &); };
struct B { operator A(); };
A foo(B x) { return x; }
  • We continue to pick the copy constructor in the following pattern:
class AutoPtrRef { };
struct AutoPtr {
  AutoPtr(AutoPtr &);
  AutoPtr();

  AutoPtr(AutoPtrRef);
  operator AutoPtrRef();
};
AutoPtr test_auto_ptr() {
  AutoPtr p;
  return p;
}

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Jul 9 2021, 7:18 PM
mizvekov edited the summary of this revision. (Show Details)Jul 9 2021, 7:20 PM
mizvekov edited the summary of this revision. (Show Details)Jul 9 2021, 7:23 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 357688.Jul 9 2021, 7:42 PM

clang-tidy.

mizvekov edited the summary of this revision. (Show Details)Jul 10 2021, 1:10 PM
mizvekov published this revision for review.Jul 10 2021, 2:39 PM
mizvekov added a reviewer: rsmith.
mizvekov added subscribers: Quuxplusone, jyknight, ldionne.
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2021, 2:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Quuxplusone accepted this revision.Jul 10 2021, 7:26 PM

It would be an excellent idea to try the libc++ test suite with this patch (make sure to pass --param std=c++03).

clang/lib/Sema/SemaStmt.cpp
3506

It sure would be nice to factor something out into a function here so that we didn't need a goto. Either replace the goto with

return PerformCopyInitialization(Entity, SourceLocation(), Value);

or else pull the Step stuff out into a static bool PermitImplicitMoveSequence(...) or whatever an appropriate name might be:

if ((Res == OR_Success || Res == OR_Deleted) && PermitImplicitMoveSequence(Seq)) {
  // Promote "AsRvalue" to the heap, etc etc
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
404

/t1/test1/?
Thanks for this note; this answers my question about whether we're testing the problematic cases. :)

clang/test/SemaObjCXX/block-capture.mm
40

cxx98_2b is just expected, isn't it?

This revision is now accepted and ready to land.Jul 10 2021, 7:26 PM
mizvekov updated this revision to Diff 358224.Jul 13 2021, 3:39 AM
mizvekov edited the summary of this revision. (Show Details)
  • Fix minor nits.
  • Please Dijkstra.
mizvekov added inline comments.Jul 13 2021, 3:40 AM
clang/test/SemaObjCXX/block-capture.mm
40

Yeah, but in this test we don't have the "expected" expectancy string.

mizvekov marked 2 inline comments as done.Jul 13 2021, 3:41 AM
This revision was automatically updated to reflect the committed changes.
NoQ added a subscriber: NoQ.Tue, Sep 7, 2:52 PM

It looks like this commit causes unexpected changes in non-C++ code. Eg., in this tiny reproducer (that uses the non-standard "blocks" feature but is otherwise plain C):

typedef struct {
  int x;
} S;

void foo() {
  ^{
    S s;
    return s;
  };
}

I'm suddenly seeing C++-specific items in the AST that shouldn't show up in C (xvalues, NRVO, etc.):

$ clang -x c -Xclang -ast-dump test.c

...
`-FunctionDecl 0x7fc55288aa88 <line:5:1, line:10:1> line:5:6 foo 'void ()'
  `-CompoundStmt 0x7fc55288aec0 <col:12, line:10:1>
    `-BlockExpr 0x7fc55288aea8 <line:6:3, line:9:3> 'S (^)(void)'
      `-BlockDecl 0x7fc55288ab70 <line:6:3, line:9:3> line:6:3
        |-CompoundStmt 0x7fc55288adc8 <col:4, line:9:3>
        | |-DeclStmt 0x7fc55288acd8 <line:7:5, col:8>
        | | `-VarDecl 0x7fc55288ac70 <col:5, col:7> col:7 used s 'S':'S' nrvo
        | `-ReturnStmt 0x7fc55288adb0 <line:8:5, col:12>
        |   `-ImplicitCastExpr 0x7fc55288ad98 <col:12> 'S':'S' <LValueToRValue>
        |     `-ImplicitCastExpr 0x7fc55288ad80 <col:12> 'S':'S' xvalue <NoOp>
        |       `-ImplicitCastExpr 0x7fc55288ad68 <col:12> 'S':'S' <LValueToRValue>
        |         `-DeclRefExpr 0x7fc55288acf0 <col:12> 'S':'S' lvalue Var 0x7fc55288ac70 's' 'S':'S'
        `-VarDecl 0x7fc55288ac70 <line:7:5, col:7> col:7 used s 'S':'S' nrvo

The double implicit lvalue-to-rvalue cast looks incorrect as well (there isn't an extra dereference here).

Here's how the old AST looks like:

...
`-FunctionDecl 0x7fb594079e88 <line:5:1, line:10:1> line:5:6 foo 'void ()'
  `-CompoundStmt 0x7fb59407a230 <col:12, line:10:1>
    `-BlockExpr 0x7fb59407a218 <line:6:3, line:9:3> 'S (^)(void)'
      `-BlockDecl 0x7fb594079f70 <line:6:3, line:9:3> line:6:3
        |-CompoundStmt 0x7fb59407a138 <col:4, line:9:3>
        | |-DeclStmt 0x7fb59407a0d8 <line:7:5, col:8>
        | | `-VarDecl 0x7fb59407a070 <col:5, col:7> col:7 used s 'S':'S'
        | `-ReturnStmt 0x7fb59407a128 <line:8:5, col:12>
        |   `-ImplicitCastExpr 0x7fb59407a110 <col:12> 'S':'S' <LValueToRValue>
        |     `-DeclRefExpr 0x7fb59407a0f0 <col:12> 'S':'S' lvalue Var 0x7fb59407a070 's' 'S':'S'
        `-VarDecl 0x7fb59407a070 <line:7:5, col:7> col:7 used s 'S':'S'

I don't think this is intended. @mizvekov, do you think you can add more hard checks to make sure it's constrained to C++? (Or maybe there's another, more precise solution?)

I don't think this is intended. @mizvekov, do you think you can add more hard checks to make sure it's constrained to C++? (Or maybe there's another, more precise solution?)

Hello! Thanks for reporting this.

The double implicit lvalue-to-rvalue cast is an unfortunate consequence of some technical debt around the different ways we handle the implicit return type on blocks.
But I don't think it's incorrect as in it actually producing program with different semantics.
Does it actually affect you in your use cases besides the AST? Is this important to you for external semantic analysis reasons or something?

Removing the xvalue cast on non CplusPlus is simple, the bigger issue here is why we are doing this, specially as it will affect what test cases we create for this change.
If this is an issue, is it serious enough to consider getting the fix into 13.0.0 at this stage?

The nrvo is a separate thing, and again I thin it's harmless, or at least a little beneficial. With it, you are avoiding an extra copy when returning these values, and it comes straight from the frontend, independent of any optimization passes later.
Why is this undesirable?

@NoQ I have made a DR for that change: D109654

NoQ added a comment.Tue, Sep 14, 8:49 PM

Hello! Thanks for reporting this.

Thanks for fixing!

The double implicit lvalue-to-rvalue cast is an unfortunate consequence of some technical debt around the different ways we handle the implicit return type on blocks.
But I don't think it's incorrect as in it actually producing program with different semantics.
Does it actually affect you in your use cases besides the AST? Is this important to you for external semantic analysis reasons or something?

I guess I'm confused about the exact meaning of the AST. I'm not sure whether it's actively incorrect or just changed meaning. I'll definitely be debugging this further.

My backstory here is that the static analyzer can be thought of as (primitivized down to) an AST "interpreter". It arranges AST nodes in the order in which they're "executed" at run-time (aka "Clang CFG") and then processes them one node at a time in that order - unlike CodeGen that processes the nodes more or less recursively. This causes us static analyzer people to have an exotic taste for the good AST as we're required to understand every node literally in isolation from other nodes. Every time the nodes make sense together/recursively but not individually our life becomes much harder.

So in the new AST (which wouldn't exist with your fix so we may be talking about vacuous truths here, as I'm still to look at the C++ variant of the same AST) I'm confused about the seemingly brand-new AST node that you introduced, ImplicitCastExpr xvalue <NoOp> - a cast that changes value kind from prvalue to glvalue and in this regard appears to behave exactly like MaterializeTemporaryExpr - which was hard enough to support the first time (due to its deep recursive nature where it extracts the storage address that was already "lost" in the operand), I really don't want to do it again :) And I'm also confused by the situation where ImplicitCastExpr <LValueToRValue> does not correspond to a memory load like it typically does.

If I understand correctly, in C++ the innermost lvalue-to-rvalue cast wouldn't exist, therefore the double memory load concern will be eliminated. I.e., it'll probably be something like

ReturnStmt
`- ImplicitCastExpr <LValueToRValue>
   `- ImplicitCastExpr <NoOp> xvalue  // MaterializeTemporaryExpr xvalue?
      `- CXXConstructExpr

which makes perfect sense to me (assuming MaterializeTemporaryExpr is actually used). I'm assuming the old AST would be

ReturnStmt
`- CXXConstructExpr

which also makes perfect sense. So if my understanding is correct then we're probably mostly good.

The nrvo is a separate thing, and again I thin it's harmless, or at least a little beneficial.

I definitely don't mind it.

Removing the xvalue cast on non CplusPlus is simple, the bigger issue here is why we are doing this, specially as it will affect what test cases we create for this change.
If this is an issue, is it serious enough to consider getting the fix into 13.0.0 at this stage?

I think your current fix is sufficient. It eliminates the known crash and we're defended against xvalues in C. I'll see what I can do to investigate this further.

I guess I'm confused about the exact meaning of the AST. I'm not sure whether it's actively incorrect or just changed meaning. I'll definitely be debugging this further.

My backstory here is that the static analyzer can be thought of as (primitivized down to) an AST "interpreter". It arranges AST nodes in the order in which they're "executed" at run-time (aka "Clang CFG") and then processes them one node at a time in that order - unlike CodeGen that processes the nodes more or less recursively. This causes us static analyzer people to have an exotic taste for the good AST as we're required to understand every node literally in isolation from other nodes. Every time the nodes make sense together/recursively but not individually our life becomes much harder.

So in the new AST (which wouldn't exist with your fix so we may be talking about vacuous truths here, as I'm still to look at the C++ variant of the same AST) I'm confused about the seemingly brand-new AST node that you introduced, ImplicitCastExpr xvalue <NoOp> - a cast that changes value kind from prvalue to glvalue and in this regard appears to behave exactly like MaterializeTemporaryExpr - which was hard enough to support the first time (due to its deep recursive nature where it extracts the storage address that was already "lost" in the operand), I really don't want to do it again :) And I'm also confused by the situation where ImplicitCastExpr <LValueToRValue> does not correspond to a memory load like it typically does.

The xvalue cast used in NRVO should never apply to a prvalue, as they are used only in expressions that refer to a variable / parameter, which are always lvalues, so it's really only changing categories while still staying within glvalues.

The NoOp cast, as far as code generation / execution is concerned, does nothing really. The purpose of a NoOp cast which changes value category lies in semantic analysis, where it will help things like deciding what kind overload will be selected or what kinds of references an expression can be bound to. Seeing those casts in the middle of a complete AST can only help make sense of why certain overloads where selected, but it would not aid in interpreting how the program should be executed.

If I understand correctly, in C++ the innermost lvalue-to-rvalue cast wouldn't exist, therefore the double memory load concern will be eliminated. I.e., it'll probably be something like

As far as the clang CodeGen is concerned, there is no double memory load here, when starting from
the left in the "LValueToRValue -> NoOp -> LValueToRValue" sequence, the code that implements this in CGExpr EmitLValue function will just recursively eat the NoOp cast (EmitCastLValue) and then go back where it started recursively calling EmitLValue again, making this equivalent to just a single LValueToRValue, as far as what code is actually emitted.

ReturnStmt
`- ImplicitCastExpr <LValueToRValue>
   `- ImplicitCastExpr <NoOp> xvalue  // MaterializeTemporaryExpr xvalue?
      `- CXXConstructExpr

I think that is not correct, in C++ when dealing with classes, the CXXConstructExpr would stand for the LValueToRValue cast there.
The implicit cast to xvalue would also stand between the DeclRef and the CXXConstruct / LValueToRValue.