[Sema] Fix volatile check when test if a return object can be implicitly move

[nullptr.cpp]

Summary

In C++11 standard, to become implicitly movable, the expression in return
statement should be a non-volatile automatic object. CWG1579 changed the rule
to require that the expression only needs to be a automatic object. C++14
standard and C++17 standard kept this rule unchanged. C++20 standard changed
the rule back to require the expression be a non-volatile automatic object.
This should be a typo in standards, and VD should be non-volatile.

[• Quuxplusone]

(This patch was split out from D88220 at my request.)

@nullptr.cpp, please add the regression test from https://godbolt.org/z/5EfK99 . After a test is added, this patch LGTM (but will need approval also from someone else).

Your summary makes it sound as if Clang's [current/old] behavior is correct in C++14 and C++17; but I think it is wrong in any version. I don't see anything in CWG1579 that affects volatile, and I see the same phrase "in a return statement in a function with a class return type, when the expression is the name of a non-volatile automatic object..." in both N3337 (C++11) and N4140 (C++14). So, I think your summary is confusing/wrong, but I think your code patch is absolutely correct: the volatile check should be moved up unconditionally, as you have done.

[nullptr.cpp]

Add test.

[nullptr.cpp]

• C++11:

[class.copy]p32:
When the criteria for elision of a copy operation are met or would be met save for the fact that the source object is a function parameter, and the object to be copied is designated by an lvalue, overload resolution to select the constructor for the copy is first performed as if the object were designated by an rvalue.

Thus should use rules at:

[class.copy]p31:
— in a return statement in a function with a class return type, when the expression is the name of a non-volatile automatic object ...

Volatile automatic objects cannot pass this check.

• CWG1579:

When the criteria for elision of a copy operation are met and the object to be copied is designated by an lvalue, or when the expression in a return statement is a (possibly parenthesized) id-expression that names an object with automatic storage duration declared in the body or parameter-declaration-clause of the innermost enclosing function or lambda-expression, overload resolution

When the expression is the name of a volatile automatic object, the first condition will fail because the criteria for elision of a copy operation require a non-volatile automatic object .
So take the ''or when ...'' branch, volatile automatic objects can pass this check.

• C++14 is same as CWG1579.

• C++17:

[class. copy. elision]p(3.1):
— If the expression in a return statement is a (possibly parenthesized) id-expression that names an object with automatic storage duration ...

Volatile automatic objects can also pass this check.

• C++20:

[class. copy. elision]p3:
An implicitly movable entity is a variable of automatic storage duration that is either a non-volatile object or an rvalue reference to a non-volatile object type.
— If the expression in a return or co_return statement is a (possibly parenthesized) id-expression that names an implicitly movable entity ...

Volatile automatic objects cannot pass this check.

[nullptr.cpp]

Ping!
I don't have commit access, can anyone help me commit this with "Yang Fan <[e-mail]>" ?
Thanks!

[nullptr.cpp]

Ping
I don't have commit access, can anyone help me commit this with "Yang Fan <[e-mail]>" ?
Thanks!

[aaronpuchert]

Can't really add anything to the discussion between @Quuxplusone and the author, just a few comments about the test.

clang/test/SemaCXX/implicitly-movable.cpp

1 ↗

(On Diff #294485)

Could you perhaps integrate this into the existing test clang/test/CXX/special/class.copy/implicit-move.cpp instead? Whenever you have something that closely corresponds to the standard, CXX is probably the right place for a test.

14 ↗

(On Diff #294485)

Is this testing what we want it to test? The private functions are just not part of the overload set, right?

I think you should make them public and = delete them.

[aaronpuchert]

clang/test/SemaCXX/implicitly-movable.cpp

14 ↗

(On Diff #294485)

Nevermind, private functions are part of the overload set, and compilation fails on master, so this is fine.

24–27 ↗

(On Diff #294485)

There is probably already a test for that, but I'll leave that to you.

[• Quuxplusone]

clang/test/SemaCXX/implicitly-movable.cpp

14 ↗

(On Diff #294485)

The private functions are just not part of the overload set, right?

Short answer "wrong." Overload resolution will make a candidate set consisting of all the signatures (even the private ones, even the deleted ones), and then pick the best match. If the best match is inaccessible (private), or if the best match is deleted, then it gives a hard error. So the idea of this test is to verify that inside test_volatile, Clang doesn't consider A(volatile A&&) to be the best match.

Making them public and deleted would neither help nor harm, in terms of correctness.

However, I agree with your above comment: it does seem like this test should actually run the code and verify that the correct overload is getting called. Right now it's unclear whether test_volatile is calling A(const volatile A&) as expected; or A(A&&); or nothing at all. (Because copy elision is weird, I think we actually expect in this case that it is doing overload resolution to require that A(const volatile A&) is callable, but then eliding the copy anyway.)

[aaronpuchert]

clang/test/SemaCXX/implicitly-movable.cpp

14 ↗

(On Diff #294485)

I think a pure frontend test is fine here.

• We're not trying to test reference initialization, and I think we can just assume that initializing an A&& with a volatile doesn't work. But I also think we can drop the test_normal case entirely, because that's probably already covered, and with it the A&& overload.
• Judging from the change title it seems to me that copy elision is not the concern here, but whether there is an implicit move or not. (Certainly they are related, but copy elision has a separate test that seems to actually check the generated IR. If copy elision is a concern, another test should probably be added to clang/test/CXX/special/class.copy/implicit-move-def.cpp.)

[nullptr.cpp]

make test more clearly

[nullptr.cpp]

In D88295#2358845, @aaronpuchert wrote:

Could you perhaps integrate this into the existing test clang/test/CXX/special/class.copy/implicit-move.cpp instead?
Whenever you have something that closely corresponds to the standard, CXX is probably the right place for a test.

clang/test/CXX/special/class.copy/implicit-move.cpp and clang/test/CXX/special/class.copy/implicit-move-def.cpp are tests for implicit (non-)declaration of move constructor and assignment, they have nothing to do with this patch.
Because copy/move elision is defined in section [class.copy.elision] in standard, so I put the test in clang/test/CXX/special/class.copy/copy-elision.cpp.

[nullptr.cpp]

rebase

[• Quuxplusone]

I believe the organization of that directory implies that the path should be class/class.init/class.copy.elision/, not special/class.copy/.
Otherwise LGTM, and I like this new test better than the old one!
I still can't help re actually landing this patch, as I don't have permissions either.

EDIT: Actually I take that back; I don't understand why any existing test would have the path special/class.copy/, since no section by that name exists in the Standard. So, I defer to anybody with a strong opinion on the matter.

[rsmith]

In D88295#2365312, @Quuxplusone wrote:

I believe the organization of that directory implies that the path should be class/class.init/class.copy.elision/, not special/class.copy/.
Otherwise LGTM, and I like this new test better than the old one!
I still can't help re actually landing this patch, as I don't have permissions either.

EDIT: Actually I take that back; I don't understand why any existing test would have the path special/class.copy/, since no section by that name exists in the Standard. So, I defer to anybody with a strong opinion on the matter.

It's historical; we haven't reorganized our tests in response to the subclauses in the standard being reorganized, and (for example) in C++98 the copy elision wording was in subclause 12.8 [class.copy], within Clause 12 [special]. Moving the test to class/class.init/class.copy.elision/p1.cpp would make sense to me. Otherwise the change to copy elision looks good; the history here is long and subtle, but the status quo after P1825R0 (which was moved as a DR) is clear that volatile objects are not implicitly movable.

However, it looks like this change also affects users of CES_AsIfByStdMove, and for "as if by std::move" semantics, I don't think volatile variables should be ignored. This affects two things:

One is whether we produce the warning suggesting use of std::move. For example, given:

X f() {
  volatile X x;
  return x;
}

... where X has a volatile copy constructor and a volatile move constructor, I think we should produce the warning suggesting use of std::move.

The other is whether implicitly move in a co_return statement. In that case, I think the use of CES_AsIfByStdMove is a mistake, and we should be using CES_Default there.

So, how about we add another CES flag to indicate whether we should reject variables with volatile-qualified type, and set it for CES_Default but not for CES_AsIfByStdMove, and also change the co_return handling to use CES_Default?

[aaronpuchert]

In D88295#2365474, @rsmith wrote:

The other is whether implicitly move in a co_return statement. In that case, I think the use of CES_AsIfByStdMove is a mistake, and we should be using CES_Default there.

I did some work on co_return in D68845 that I should probably warm up again. I think the issue still exists.

We agreed there that we should use CES_Default. Unfortunately it was a little bit more difficult though, and we were not sure whether the standard wants to say the things it says at some point.

clang/test/CXX/special/class.copy/copy-elision.cpp

1–4 ↗

(On Diff #301822)

Since the issue doesn't depend on the standard version I guess that one execution (without -std= flag) is enough, but maybe others have a different opinion.

[• Quuxplusone]

So, how about we add another CES flag

As the original author of CES_AsIfByStdMove, I am opposed to any attempt to complicate this patch. My medium-term goal, now that P1155 has been adopted, is to eliminate the complexity around -Wreturn-std-move diagnostics. I want to be able to say, "This stuff got fixed in the paper standard and now Clang doesn't need to diagnose it anymore."

If we want to do CES correctly, I think we should make One Big Patch that supports implicit move for co_return, and support P0527 "implicitly move from rvalue references," and supports the P2025 "guaranteed NRVO" optimizations while we're at it. I encouraged Yang to split out this tiny bugfix into a separate patch precisely because it is a tiny bugfix. I would push it right now if I had permission to.

[aaronpuchert]

In D88295#2365474, @rsmith wrote:

... where X has a volatile copy constructor and a volatile move constructor, I think we should produce the warning suggesting use of std::move.

If I read this correctly, we'd have a false negative with this patch, which is probably Ok given that this is an odd case.

In D88295#2365560, @Quuxplusone wrote:

that supports implicit move for co_return

Don't we already have that with D51741? But it's easy to get confused here.

I would push it right now if I had permission to.

I don't read @rsmith's comment as a strong objection. Let's give @nullptr.cpp a chance to react to the suggestions both of you had about where to place the test (sorry about my silly suggestions earlier, I just grepped around and didn't read it). Then I think this is fine.

[nullptr.cpp]

move test to class/class.init/class.copy.elision/p1.cpp

[rsmith]

In D88295#2365560, @Quuxplusone wrote:

So, how about we add another CES flag

As the original author of CES_AsIfByStdMove, I am opposed to any attempt to complicate this patch. My medium-term goal, now that P1155 has been adopted, is to eliminate the complexity around -Wreturn-std-move diagnostics. I want to be able to say, "This stuff got fixed in the paper standard and now Clang doesn't need to diagnose it anymore."

OK, I think that's reasonable. I don't think it's worth keeping -Wreturn-std-move around if the only time it ever fires is when returning a volatile-qualified local variable; it certainly does not pull its weight if it only diagnoses that situation. I'm happy with losing the diagnostic for that case now, especially if we intend to remove the warning entirely as a follow-up.

(The changes to co_return are a bugfix, so that seems fine. I'm not going to ask for a test for that change; it seems preferable to leave that to D68845 or wherever we properly implement the copy-elision rules for co_return.)

[nullptr.cpp]

Thanks for all your suggestions!
But I don't have commit access, can anyone help me commit this with "Yang Fan <[e-mail]>" ?
Thanks!

clang/lib/Sema/SemaStmt.cpp

bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
 
   // Return false if VD is a __block variable. We don't want to implicitly move
   // out of a __block variable during a return because we cannot assume the
   // variable will no longer be used.
   if (VD->hasAttr<BlocksAttr>()) return false;
 
+  // ...non-volatile...
+  if (VD->getType().isVolatileQualified())
+    return false;
+
   if (CESK & CES_AllowDifferentTypes)
     return true;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified()) return false;
-
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr<AlignedAttr>() &&
       Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VD->getType()))
     return false;
 
   return true;
 }

clang/test/CXX/class/class.init/class.copy.elision/p1.cpp

+// RUN: %clang_cc1 -std=c++20 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+
+// - volatile object in return statement don't match the rule for using move
+//   operation instead of copy operation. Thus should call the copy constructor
+//   A(const volatile A &).
+//
+// - volatile object in return statement also don't match the rule for copy
+//   elision. Thus the copy constructor A(const volatile A &) cannot be elided.
+namespace test_volatile {
+class A {
+public:
+  A() {}
+  ~A() {}
+  A(const volatile A &);
+  A(volatile A &&);
+};
+
+A test() {
+  volatile A a_copy;
+  // CHECK: call void @_ZN13test_volatile1AC1ERVKS0_
+  return a_copy;
+}
+} // namespace test_volatile