Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

[• Quuxplusone]

Summary

This patch adds two new diagnostics, which are off by default:

-Wreturn-std-move

This diagnostic is enabled by -Wreturn-std-move, -Wmove, or -Wall.
Diagnose cases of return x or throw x, where x is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is stdext::inplace_function<Sig, N> which implements conversion via a conversion operator rather than a converting constructor; see https://github.com/WG21-SG14/SG14/issues/125#issue-297201412
Another place where this has come up in the wild, but where the fix ended up being different, was

try { ... } catch (ExceptionType ex) {
    throw ex;
}

where the appropriate fix in that case was to replace throw ex; with throw;, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

-Wreturn-std-move-in-c++11

This diagnostic is enabled only by the exact spelling -Wreturn-std-move-in-c++11.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

Discussion

Notice that this patch is kind of a negative-space version of Richard Trieu's -Wpessimizing-move. That diagnostic warns about cases of return std::move(x) that should be return x for speed. These diagnostics warn about cases of return x that should be return std::move(x) for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a return statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user would see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit std::move only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

23

Can we say something like "local variable '%0' will be copied despite being %select{returned|thrown}1 by name; call 'std::move' explicitly to avoid the copy"? (Would that be accurate, given the implementation of the warning?)

Ideally, we'd move the "call 'std::move' explicitly" hint to a separate note diagnostic and include a FixItHint on that diagnostic to insert the call to std::move.

27

I think the wording of this diagnostic could be improved too. It's very language-lawyer-friendly right now. Here's one possible starting point for alternative wording (though I think maybe we can do better):

"ISO C++11, prior to the resolution of defect reports, would have made a copy of local variable '%0' despite it being returned by name due to a type mismatch with the return value%diff{ ($ vs $)|}1, 2"

... plus a "note: call 'std::move' explicitly to avoid a copy when using older compilers" or similar, with a fixit.

[• Quuxplusone]

include/clang/Basic/DiagnosticSemaKinds.td

23

SGTM, except that I don't know how to find out whether we're in the context of a return or a throw from this deep in the guts of Sema. Could someone give me a pointer on that?

I also had trouble figuring out how to generate a fixit from x to std::move(x) — it keeps coming out as std::move( ) — but I expect I'll be able to solve that one even without help by banging my head against it a bit.

include/clang/Sema/Sema.h

3827

Q: Is it appropriate for me to be changing the signature of this public method at all? I don't know libclang's rules regarding this kind of change.

lib/Sema/SemaExprCXX.cpp

731 ↗

(On Diff #134327)

Q: I'm not sure what concept is being represented by "CES_Strict" here. If there is a more standardese-appropriate name, I'd like to use it. And if there are multiple concepts here that just happen to be identical *coincidentally*, I'd probably prefer to use multiple enumerators that all just happen to have value 0. But because there are so many call-sites and because this is not directly related to my patch, I have not investigated much.

[• Quuxplusone]

Reword the diagnostic per rsmith's suggestions.
Still TODO:

• figure out how to detect whether this is a return-stmt or throw-expression
• figure out how to generate the appropriate fixit

[• Quuxplusone]

Add fixits.

[• Quuxplusone]

@rsmith and/or @rtrieu, please take another look? All my TODOs are done now: there are fixits, and the wording of the diagnostic changes if it's a "throw" instead of a "return", and the wording has been updated per Richard Smith's suggestions.

I have one very minor nit that I don't know how to fix:

warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied
      despite being returned by name [-Wreturn-std-move]
    return (d);  // e17
           ^
warn-return-std-move.cpp:220:12: note: call 'std::move' explicitly to avoid copying
    return (d);  // e17
           ^~~
           std::move(d)

The warning places a caret directly under the (, when I wish it would place ^~~ under the entire expression, the way the fixit does.

I also spent a little time looking into whether I could/should diagnose

auto [x, y] = std::make_pair(Derived(), Derived());
return x;  // 'x' will be copied despite being returned by name

but I have decided that this is probably too far afield to be rolled into this patch, even if I could figure out how to detect it, which to a first approximation I cannot. So I am deliberately punting on that one.

[• Quuxplusone]

Removed a redundant check for LValueReferenceType in the CWG1579 codepath. (In that branch, we know that standard C++ *did* perform the copy-to-move transformation, so obviously we can't have had an lvalue reference type originally.) I've verified that the check is still needed (not redundant) along the other codepath.

[• Quuxplusone]

Eliminate a couple of auto per comment by @Rakete1111.

[lebedev.ri]

Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV
I wonder whether that also should be diagnosed? Where though, as clang-tidy check?

[• Quuxplusone]

In D43322#1017190, @lebedev.ri wrote:

Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV

IIUC, you're asking whether it would be possible to detect instances of

return take(mysharedptr);

and rewrite them into

return take(std::move(mysharedptr));

(Notice that the "return" context is relatively important, because if mysharedptr is used again after this use, then the move-from is unsafe and should not be suggested. Of course it's always possible for mysharedptr to be used again after a return or throw as well, e.g. during stack unwinding/destructors; but there's precedent there for C++ saying we'll take that chance.)

If you start treating names as rvalues no matter where they appear in larger expressions, then you run into trouble with

std::string hello = ...;
std::string &hello2 = hello;
return concatenate(hello, hello2);

where the first use of hello on this line moves-out-of it, and then the second use reads the moved-from value. My best guess is that things like that occur frequently enough in real-world code that (A) the Standard can't specify move behavior there because it would quietly break code, and (B) a Clang diagnostic would produce mostly false positives (in the sense that accepting the fixits would quietly break the user's code). I admit it would be interesting to write the code and find out empirically.

Whereas in this specific limited case of return x; and throw x;, programmers are already trained to expect a move, and there are no (new) aliasing issues.

Where though, as clang-tidy check?

My experience with this patch suggests that when the thing you need to do involves "run a hypothetical overload-resolution," it is basically impossible to do with clang-tidy, and relatively easy to do in clang proper. (I tried writing this check into clang-tidy first, and couldn't figure out how to do the overload resolution part. Whereas it was painless in clang, especially with -Wpessimizing-move to copy off of.)

[• Quuxplusone]

@rtrieu please take a look?

[rtrieu]

I have one very minor nit that I don't know how to fix:

warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied
      despite being returned by name [-Wreturn-std-move]
    return (d);  // e17
           ^
warn-return-std-move.cpp:220:12: note: call 'std::move' explicitly to avoid copying
    return (d);  // e17
           ^~~
           std::move(d)

The warning places a caret directly under the (, when I wish it would place ^~~ under the entire expression, the way the fixit does.

You can add extra tildes to any diagnostic by passing a SourceRange to Diag() calls the same way you pass FixitHints.

lib/Sema/SemaStmt.cpp

2937

I have a few concerns about this function. The code isn't a straight move from the old location to this one, so it is hard to follow along the changes, especially the change to the complex if conditional. The function should be commented, especially to behavior difference for setting IsFake.

It looks like when IsFake is set, then the result is discarded and not used, but it is still possible on success for AsRvalue to be copied to the heap. This is wasteful when it is never used again.

Another issue is the Value in the original code is used again towards the end of the function on line #3013. In the old code, Value can be updated while in the new code, it does.

It may be better to split this change in two, the first adding this function and the CopyElisionSemanticsKind enum and the second adding the diagnostic itself.

[• Quuxplusone]

Rename some functions and parameters. Rebase onto D43898.

lib/Sema/SemaStmt.cpp

2937

Hi @rtrieu, and thanks!

I have split out the first half of the patch into a new revision D43898, and updated this one with the full patch (both halves together). Is there an easy way for me to make "just the second half" reviewable on its own, before the first half has been merged to master?

It looks like when IsFake is set, then the result is discarded and not used, but it is still possible on success for AsRvalue to be copied to the heap. This is wasteful when it is never used again.

I believe you are correct. But I'm not sure if it's safe to use AsRvalue as input to Res (which *is* used outside this function) if it's not moved like this; I don't know much about the internals here. You or anyone have a suggestion for how to fix that issue?

In the old code, Value can be updated while in the new code, it does.

I can't parse this, sorry.

FYI, in the patch I'm about to upload, I have renamed !IsFake to ConvertingConstructorsOnly, which should be more pleasing to the eye. :)

[• Quuxplusone]

include/clang/Basic/DiagnosticSemaKinds.td

5639

I would like some guidance on whether it would be appropriate to turn this warning on as part of -Wmove.

lib/Sema/SemaStmt.cpp

3083

This source range does the right thing; thanks @rtrieu!

[• Quuxplusone]

@rtrieu (and perhaps @rsmith) ping?

The action items I need help with are:

• Review and land the refactoring patch D43898 (I don't have commit privs)
• Ideally, test compiling a bunch of (e.g. Google) code with D43322, see if there are any rough edges
• Decide whether -Wmove should imply -Wreturn-std-move
• Review and land the new-diagnostic patch D43322
• Ideally, some discussion of whether I should write a paper for San Diego proposing that C++ *should* move rather than copy in these cases, and whether that possibility should change anything about this patch

Speaking of rough edges, I did find that in my employer's codebase the far-and-away-most-common trigger is return my_shared_ptr_to_derived_class; in a function that is declared to return shared_ptr<base_class>. [EDIT: oops, I forgot that this hits only -Wreturn-std-move-in-c++11, which shouldn't be included in -Wmove anyway.] This does indeed generate an extra atomic increment/decrement compared to the optimal code, but I could imagine someone considering that a "false positive."
Other than that one questionable scenario, everything else I've found with it so far has been a true positive. There are no positives (either true or false) in LLVM's codebase.

[• Quuxplusone]

Btw, I'm going to be talking about this patch tonight at 7 in Palo Alto. :)
https://www.meetup.com/ACCU-Bay-Area/events/248040207/
https://docs.google.com/presentation/d/18ZRnedocXSQKn9Eh67gGv-ignReHfRD7vj_dxrra1kc/

[• Quuxplusone]

Rebased on D43898 after addressing @rtrieu's latest comments over there.

[• Quuxplusone]

Boldly add -Wreturn-std-move to -Wmove (and thus also to -Wall).
The backward-compatibility-concerned diagnostic, -Wreturn-std-move-in-c++11, is *not* appropriate for -Wmove; but I believe this main diagnostic is appropriate.

[lebedev.ri]

In D43322#1037889, @Quuxplusone wrote:

The backward-compatibility-concerned diagnostic, -Wreturn-std-move-in-c++11, is *not* appropriate for -Wmove;

Have you evaluated possibility of adding -Wreturn-std-move-in-c++11 to one of CXX..Compat groups?

[• Quuxplusone]

include/clang/Basic/DiagnosticGroups.td

388

The backward-compatibility-concerned diagnostic, -Wreturn-std-move-in-c++11, is *not* appropriate for -Wmove;

Have you evaluated possibility of adding -Wreturn-std-move-in-c++11 to one of CXX..Compat groups?

Hmm. I've considered it, but I don't know what the appropriate action is...
I suspect that it should *not* be "CXX11Compat" because technically the change was a DR against C++11 — sadly this is about portability to "older" compilers, not portability to "lesser" C++ standards.
Of course it can't be "CXX98Compat", for several reasons. :)
I'm not sure of the purpose of the "CXX..CompatPedantic" groups. They are all identical to the "CXX..Compat" groups, except that CXX98CompatPedantic includes one extra warning about binding to lifetime-extended temporaries which don't have copy constructors.

[• Quuxplusone]

Rebase on master, now that the refactoring D43898 has been pushed (thanks @rtrieu!)

[• Quuxplusone]

@rtrieu ping! I've rebased this on the pure refactoring you committed for me last week.

[• Quuxplusone]

@rtrieu gentle ping!

Action items I need help with, cut-and-pasted from above:

• Ideally, test compiling a bunch of (e.g. Google) code with D43322, see if there are any rough edges
• Decide if -Wmove should imply -Wreturn-std-move (I have preemptively done this)
• Review and land D43322
• Ideally, some discussion of whether I should write a paper for San Diego proposing that C++ should move rather than copy in these cases (my default plan is to do this)

[• Quuxplusone]

Rebased on master. AFAIK this is ready to go and I just need someone to land it for me... @rtrieu ping?

I believe the PR summary would be suitable as a commit message.

[rsmith]

include/clang/Sema/Sema.h

3806

Please wrap to 80 columns.

3827

For reference, changes here are fine. We provide no API or ABI stability guarantees for our C++ ABI; libclang provides an ABI-stable C ABI layered on top of our internal API for this reason.

lib/Sema/SemaStmt.cpp

3075–3076

I've seen this a lot more for returning unique_ptr<Derived> from a function with return type unique_ptr<Base>. Maybe just give this Expected<T> case as an example rather than saying it's the most common example?

[• Quuxplusone]

Addressed review comments from @rsmith and rebased on master.
I still need someone else to land this for me, though.

[• Quuxplusone]

@rsmith ping?

[• Quuxplusone]

Finally learned how to "make check-clang" and actually run the test in the correct environment. Had to add -fcxx-exceptions to the command lines at the top of that file, because the code uses throw.

@rsmith PTAL?

[• Quuxplusone]

Tuesday ping! I just need someone to commit this for me.

[thakis]

This is a cool warning, thanks for adding it. We ran into one thing while enabling this in Chromium that I'd like to mention here. We have code that basically does:

struct Foo {
  using passwords_iterator = std::map<base::string16,
                                      std::set<std::string>,
                                      ReverseStringLess>::const_iterator;
  std::map<base::string16, std::set<std::string>, ReverseStringLess> passwords_;
  passwords_iterator get(const base::string16& in) {
    auto it = passwords_.lower_bound(in);
    return it;
  }
};

Here, the warning gets emitted because auto it becomes a non-const iterator, and passwords_iterator is a const_iterator. Maybe the warning could suggest something like "cv qualifiers don't match, make them match" on a note in addition or instead of std::move() for this case?

And then someone else pointed out that http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579 might mean that the code-as is should be fine too in C++14 – I don't know if that's true, but maybe you could comment on that too :-)

(references:
https://cs.chromium.org/chromium/src/components/password_manager/core/browser/password_reuse_detector.h?type=cs&q=passwords_iterator&sq=package:chromium&l=64
https://chromium-review.googlesource.com/c/chromium/src/+/1025435/6/components/password_manager/core/browser/password_reuse_detector.cc
https://chromium-review.googlesource.com/c/chromium/src/+/1025435/8/components/password_manager/core/browser/password_reuse_detector.cc
)

[• arthur.j.odwyer]

Sorry, I responded via email but I guess Phabricator didn't pick it up for
some reason.
See below.

[• Quuxplusone]

In D43322#1083075, @arthur.j.odwyer wrote:

Sorry, I responded via email but I guess Phabricator didn't pick it up for
some reason.
See below.

And then Phab *still* didn't pick up the important part... Okay, I'll try pasting it here.

Can you post the diagnostic exactly as it appears in the compiler output? I am surprised that it would appear here. It should appear here only if the standard library's implicit conversion from std::map<...>::iterator to std::map<...>::const_iterator were implemented as a conversion operator instead of as a converting constructor. I have verified that both libstdc++ trunk and libc++ trunk implement it "correctly" as a converting constructor, and I have verified on Wandbox/Godbolt that no warning is emitted on your sample code when using either of those standard libraries.

Is it possible that you are using a standard library that is neither libc++ or libstdc++?
Is it possible that that standard library implements the iterator-to-const_iterator conversion as a conversion operator instead of a converting constructor?

And then someone else pointed out that http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579 might mean that the code-as is should be fine too in C++14 – I don't know if that's true, but maybe you could comment on that too :-)

CWG1579 is the defect report against ISO C++11 which is mentioned in the wording of the off-by-default -Wreturn-std-move-in-c++11. :) Anything that -Wreturn-std-move itself complains about, is stuff that was NOT fixed by CWG1579.
Alternative hypothesis: Is it possible that you have manually turned on -Wreturn-std-move-in-c++11 (which is deliberately omitted from -Wmove/-Wall)? In that case, the warning is absolutely correct, but you are probably wrong to care about fixing it in this case. :)

Maybe the warning could suggest something like "cv qualifiers don't match, make them match" on a note in addition or instead of std::move() for this case?

I would love to see this, but I'm not comfortable trying to add it myself. Remember, the problem here isn't core-language cv-qualifiers ("iterator" vs. "const iterator"); the problem is library-level type mismatch ("iterator" vs. "const_iterator"). You'd have to somehow look at the human-readable name of the typedef and determine that it was a match for the regex /const_(.*)/ and the target type was a match for /\1/, and then you'd have to generate a meaningful note.

[• Quuxplusone]

Can you post the diagnostic exactly as it appears in the compiler output? I am surprised that it would appear here. It should appear here only if the standard library's implicit conversion from std::map<...>::iterator to std::map<...>::const_iterator were implemented as a conversion operator instead of as a converting constructor. I have verified that both libstdc++ trunk and libc++ trunk implement it "correctly" as a converting constructor, and I have verified on Wandbox/Godbolt that no warning is emitted on your sample code when using either of those standard libraries.

Is it possible that you are using a standard library that is neither libc++ or libstdc++?

Is it possible that that standard library implements the iterator-to-const_iterator conversion as a conversion operator instead of a converting constructor?

@thakis ping — did you ever resolve this issue to your satisfaction?

include/clang/Basic/DiagnosticGroups.td

 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
+
 def PessimizingMove : DiagGroup<"pessimizing-move">;
+def ReturnStdMoveInCXX11 : DiagGroup<"return-std-move-in-c++11">;
+def ReturnStdMove : DiagGroup<"return-std-move">;
+
 def PointerArith : DiagGroup<"pointer-arith">;
 def PoundWarning : DiagGroup<"#warnings">;
 def PoundPragmaMessage : DiagGroup<"#pragma-messages">,
                          DiagCategory<"#pragma message Directive">;
 def : DiagGroup<"pointer-to-int-cast">;
 def : DiagGroup<"redundant-decls">;
 def RedeclaredClassMember : DiagGroup<"redeclared-class-member">;
 def GNURedeclaredEnum : DiagGroup<"gnu-redeclared-enum">;

 def TypeSafety : DiagGroup<"type-safety">;
 
 def IncompatibleExceptionSpec : DiagGroup<"incompatible-exception-spec">;
 
 def IntToVoidPointerCast : DiagGroup<"int-to-void-pointer-cast">;
 def IntToPointerCast : DiagGroup<"int-to-pointer-cast",
                                  [IntToVoidPointerCast]>;
 
-def Move : DiagGroup<"move", [PessimizingMove, RedundantMove, SelfMove]>;
+def Move : DiagGroup<"move", [
+    PessimizingMove,
+    RedundantMove,
+    ReturnStdMove,
+    SelfMove
+  ]>;
 
 def Extra : DiagGroup<"extra", [
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,

include/clang/Basic/DiagnosticSemaKinds.td

 let CategoryName = "Semantic Issue" in {
 
 def note_previous_decl : Note<"%0 declared here">;
 def note_entity_declared_at : Note<"%0 declared here">;
 def note_callee_decl : Note<"%0 declared here">;
 def note_defined_here : Note<"%0 defined here">;
 
 // For loop analysis
 def warn_variables_not_in_loop_body : Warning<
   "variable%select{s| %1|s %1 and %2|s %1, %2, and %3|s %1, %2, %3, and %4}0 "
   "used in loop condition not modified in loop body">,
   InGroup<ForLoopAnalysis>, DefaultIgnore;
 def warn_redundant_loop_iteration : Warning<
   "variable %0 is %select{decremented|incremented}1 both in the loop header "
   "and in the loop body">,
   InGroup<ForLoopAnalysis>, DefaultIgnore;
 def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">;
 
 def warn_duplicate_enum_values : Warning<
   "element %0 has been implicitly assigned %1 which another element has "
   "been assigned">, InGroup<DiagGroup<"duplicate-enum">>, DefaultIgnore;

 def warn_pessimizing_move_on_return : Warning<
   "moving a local object in a return statement prevents copy elision">,
   InGroup<PessimizingMove>, DefaultIgnore;
 def warn_pessimizing_move_on_initialization : Warning<
   "moving a temporary object prevents copy elision">,
   InGroup<PessimizingMove>, DefaultIgnore;
 def note_remove_move : Note<"remove std::move call here">;
 
+def warn_return_std_move : Warning<
+  "local variable %0 will be copied despite being %select{returned|thrown}1 by name">,
+  InGroup<ReturnStdMove>, DefaultIgnore;
+def note_add_std_move : Note<
+  "call 'std::move' explicitly to avoid copying">;
+def warn_return_std_move_in_cxx11 : Warning<
+  "prior to the resolution of a defect report against ISO C++11, "
+  "local variable %0 would have been copied despite being returned by name, "
+  "due to its not matching the function return type%diff{ ($ vs $)|}1,2">,
+  InGroup<ReturnStdMoveInCXX11>, DefaultIgnore;
+def note_add_std_move_in_cxx11 : Note<
+  "call 'std::move' explicitly to avoid copying on older compilers">;
+
 def warn_string_plus_int : Warning<
   "adding %0 to a string does not append to the string">,
   InGroup<StringPlusInt>;
 def warn_string_plus_char : Warning<
   "adding %0 to a string pointer does not append to the string">,
   InGroup<StringPlusChar>;
 def note_string_plus_scalar_silence : Note<
   "use array indexing to silence this warning">;
 
 def warn_sizeof_array_param : Warning<
   "sizeof on array function parameter will return size of %0 instead of %1">,
   InGroup<SizeofArrayArgument>;
 
 def warn_sizeof_array_decay : Warning<

include/clang/Sema/Sema.h

public:
   RecordDecl *CreateCapturedStmtRecordDecl(CapturedDecl *&CD,
                                            SourceLocation Loc,
                                            unsigned NumParams);
 
   enum CopyElisionSemanticsKind {
     CES_Strict = 0,
     CES_AllowParameters = 1,
     CES_AllowDifferentTypes = 2,
+    CES_AllowExceptionVariables = 4,
+    CES_FormerDefault = (CES_AllowParameters),
     CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes),
+    CES_AsIfByStdMove = (CES_AllowParameters | CES_AllowDifferentTypes |
+                         CES_AllowExceptionVariables),
   };
 
   VarDecl *getCopyElisionCandidate(QualType ReturnType, Expr *E,
                                    CopyElisionSemanticsKind CESK);
   bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
                               CopyElisionSemanticsKind CESK);
 
   StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                              Scope *CurScope);
   StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
   StmtResult ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
 
   StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                              bool IsVolatile, unsigned NumOutputs,
                              unsigned NumInputs, IdentifierInfo **Names,
                              MultiExprArg Constraints, MultiExprArg Exprs,
                              Expr *AsmString, MultiExprArg Clobbers,
                              SourceLocation RParenLoc);
 
   void FillInlineAsmIdentifierInfo(Expr *Res,
                                    llvm::InlineAsmIdentifierInfo &Info);
   ExprResult LookupInlineAsmIdentifier(CXXScopeSpec &SS,
                                        SourceLocation TemplateKWLoc,
                                        UnqualifiedId &Id,
                                        bool IsUnevaluatedContext);
   bool LookupInlineAsmField(StringRef Base, StringRef Member,
                             unsigned &Offset, SourceLocation AsmLoc);
   ExprResult LookupInlineAsmVarDeclField(Expr *RefExpr, StringRef Member,

lib/Sema/SemaStmt.cpp

if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
         !Context.hasSameUnqualifiedType(ReturnType, VDType))
       return false;
   }
 
   // ...object (other than a function or catch-clause parameter)...
   if (VD->getKind() != Decl::Var &&
       !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
     return false;
-  if (VD->isExceptionVariable()) return false;
+  if (!(CESK & CES_AllowExceptionVariables) && VD->isExceptionVariable())
+    return false;
 
   // ...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;
if (!VD->getType()->isDependentType() && VD->hasAttr<AlignedAttr>() &&
     return false;
 
   return true;
 }
 
 /// \brief Try to perform the initialization of a potentially-movable value,
 /// which is the operand to a return or throw statement.
 ///
 /// This routine implements C++14 [class.copy]p32, which attempts to treat
 /// returned lvalues as rvalues in certain cases (to prefer move construction),
 /// then falls back to treating them as lvalues if that failed.
 ///
+/// \param ConvertingConstructorsOnly If true, follow [class.copy]p32 and reject
+/// resolutions that find non-constructors, such as derived-to-base conversions
+/// or `operator T()&&` member functions. If false, do consider such
+/// conversion sequences.
+///
 /// \param Res We will fill this in if move-initialization was possible.
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
 static void TryMoveInitialization(Sema& S,
                                   const InitializedEntity &Entity,
                                   const VarDecl *NRVOCandidate,
                                   QualType ResultType,
                                   Expr *&Value,
+                                  bool ConvertingConstructorsOnly,
                                   ExprResult &Res) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                             CK_NoOp, Value, VK_XValue);
 
   Expr *InitExpr = &AsRvalue;
 
   InitializationKind Kind = InitializationKind::CreateCopy(
       Value->getLocStart(), Value->getLocStart());
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
   if (!Seq)
     return;
 
   for (const InitializationSequence::Step &Step : Seq.steps()) {
     if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
         Step.Kind != InitializationSequence::SK_UserConversion)
       continue;
 
     FunctionDecl *FD = Step.Function.Function;
+    if (ConvertingConstructorsOnly) {
       if (isa<CXXConstructorDecl>(FD)) {
         // C++14 [class.copy]p32:
         // [...] If the first overload resolution fails or was not performed,
         // or if the type of the first parameter of the selected constructor
         // is not an rvalue reference to the object's type (possibly
         // cv-qualified), overload resolution is performed again, considering
         // the object as an lvalue.
         const RValueReferenceType *RRefType =
             FD->getParamDecl(0)->getType()->getAs<RValueReferenceType>();
         if (!RRefType)
           break;
         if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
                                               NRVOCandidate->getType()))
           break;
       } else {
         continue;
       }
+    } else {
+      if (isa<CXXConstructorDecl>(FD)) {
+        // Check that overload resolution selected a constructor taking an
+        // rvalue reference. If it selected an lvalue reference, then we
+        // didn't need to cast this thing to an rvalue in the first place.
+        if (!isa<RValueReferenceType>(FD->getParamDecl(0)->getType()))
+          break;
+      } else if (isa<CXXMethodDecl>(FD)) {
+        // Check that overload resolution selected a conversion operator
+        // taking an rvalue reference.
+        if (cast<CXXMethodDecl>(FD)->getRefQualifier() != RQ_RValue)
+          break;
+      } else {
+        continue;
+      }
+    }
 
     // Promote "AsRvalue" to the heap, since we now need this
     // expression node to persist.
     Value = ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp,
                                      Value, nullptr, VK_XValue);
 
     // Complete type-checking the initialization of the return type
     // using the constructor we found.
Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
   // 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 to
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
 
   if (AllowNRVO) {
+    bool AffectedByCWG1579 = false;
+
     if (!NRVOCandidate) {
       NRVOCandidate = getCopyElisionCandidate(ResultType, Value, CES_Default);
+      if (NRVOCandidate &&
+          !getDiagnostics().isIgnored(diag::warn_return_std_move_in_cxx11,
+                                      Value->getExprLoc())) {
+        const VarDecl *NRVOCandidateInCXX11 =
+            getCopyElisionCandidate(ResultType, Value, CES_FormerDefault);
+        AffectedByCWG1579 = (!NRVOCandidateInCXX11);
+      }
     }
 
     if (NRVOCandidate) {
       TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-                            Res);
+                            true, Res);
+    }
+
+    if (!Res.isInvalid() && AffectedByCWG1579) {
+      QualType QT = NRVOCandidate->getType();
+      if (QT.getNonReferenceType()
+                     .getUnqualifiedType()
+                     .isTriviallyCopyableType(Context)) {
+        // Adding 'std::move' around a trivially copyable variable is probably
+        // pointless. Don't suggest it.
+      } else {
+        // Common cases for this are returning unique_ptr<Derived> from a
+        // function of return type unique_ptr<Base>, or returning T from a
+        // function of return type Expected<T>. This is totally fine in a
+        // post-CWG1579 world, but was not fine before.
+        assert(!ResultType.isNull());
+        SmallString<32> Str;
+        Str += "std::move(";
+        Str += NRVOCandidate->getDeclName().getAsString();
+        Str += ")";
+        Diag(Value->getExprLoc(), diag::warn_return_std_move_in_cxx11)
+            << Value->getSourceRange()
+            << NRVOCandidate->getDeclName() << ResultType << QT;
+        Diag(Value->getExprLoc(), diag::note_add_std_move_in_cxx11)
+            << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
+      }
+    } else if (Res.isInvalid() &&
+               !getDiagnostics().isIgnored(diag::warn_return_std_move,
+                                           Value->getExprLoc())) {
+      const VarDecl *FakeNRVOCandidate =
+          getCopyElisionCandidate(QualType(), Value, CES_AsIfByStdMove);
+      if (FakeNRVOCandidate) {
+        QualType QT = FakeNRVOCandidate->getType();
+        if (QT->isLValueReferenceType()) {
+          // Adding 'std::move' around an lvalue reference variable's name is
+          // dangerous. Don't suggest it.
+        } else if (QT.getNonReferenceType()
+                       .getUnqualifiedType()
+                       .isTriviallyCopyableType(Context)) {
+          // Adding 'std::move' around a trivially copyable variable is probably
+          // pointless. Don't suggest it.
+        } else {
+          ExprResult FakeRes = ExprError();
+          Expr *FakeValue = Value;
+          TryMoveInitialization(*this, Entity, FakeNRVOCandidate, ResultType,
+                                FakeValue, false, FakeRes);
+          if (!FakeRes.isInvalid()) {
+            bool IsThrow =
+                (Entity.getKind() == InitializedEntity::EK_Exception);
+            SmallString<32> Str;
+            Str += "std::move(";
+            Str += FakeNRVOCandidate->getDeclName().getAsString();
+            Str += ")";
+            Diag(Value->getExprLoc(), diag::warn_return_std_move)
+                << Value->getSourceRange()
+                << FakeNRVOCandidate->getDeclName() << IsThrow;
+            Diag(Value->getExprLoc(), diag::note_add_std_move)
+                << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
+          }
+        }
+      }
     }
   }
 
   // Either we didn't meet the criteria for treating an lvalue as an rvalue,
   // above, or overload resolution failed. Either way, we need to try
   // (again) now with the return value expression as written.
   if (Res.isInvalid())
     Res = PerformCopyInitialization(Entity, SourceLocation(), Value);

test/SemaCXX/warn-return-std-move.cpp

+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -Wreturn-std-move -Wreturn-std-move-in-c++11 -std=c++14 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -Wreturn-std-move -Wreturn-std-move-in-c++11 -std=c++14 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template <class T> struct remove_reference { typedef T type; };
+template <class T> struct remove_reference<T&> { typedef T type; };
+template <class T> struct remove_reference<T&&> { typedef T type; };
+
+template <class T> typename remove_reference<T>::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+    Instrument() {}
+    Instrument(Instrument&&) { /* MOVE */ }
+    Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+    Instrument i;
+    operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+    operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+    operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+    operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+    Instrument i;
+    ConstructFromBase(const Base& b): i(b.i) {}
+    ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+    Instrument i;
+    ConstructFromDerived(const Derived& d): i(d.i) {}
+    ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+    int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+    TrivialInstrument i;
+    operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+    operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+    operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+    operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+    TrivialInstrument i;
+    ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+    ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+    TrivialInstrument i;
+    ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+    ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+    Derived d1;
+    return d1;  // ok
+}
+Base test2() {
+    Derived d2;
+    return d2;  // e1
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+    Derived d3;
+    return d3;  // e2-cxx11
+    // expected-warning@-1{{would have been copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying on older compilers}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+    Derived d4;
+    return d4;  // e3
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+    Derived d5;
+    return d5;  // e4
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+    Derived d6;
+    return d6;  // e5
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast<Derived&&>(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast<Derived&&>(d); }
+ConstructFromBase ok4() { Derived d; return static_cast<Derived&&>(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast<Derived&&>(d); }
+ConvertFromBase ok6() { Derived d; return static_cast<Derived&&>(d); }
+
+// If the target is an lvalue reference, assume it's not safe to move from.
+Derived ok_plvalue1(Derived& d) { return d; }
+Base ok_plvalue2(Derived& d) { return d; }
+ConstructFromDerived ok_plvalue3(const Derived& d) { return d; }
+ConstructFromBase ok_plvalue4(Derived& d) { return d; }
+ConvertFromDerived ok_plvalue5(Derived& d) { return d; }
+ConvertFromBase ok_plvalue6(Derived& d) { return d; }
+
+Derived ok_lvalue1(Derived *p) { Derived& d = *p; return d; }
+Base ok_lvalue2(Derived *p) { Derived& d = *p; return d; }
+ConstructFromDerived ok_lvalue3(Derived *p) { const Derived& d = *p; return d; }
+ConstructFromBase ok_lvalue4(Derived *p) { Derived& d = *p; return d; }
+ConvertFromDerived ok_lvalue5(Derived *p) { Derived& d = *p; return d; }
+ConvertFromBase ok_lvalue6(Derived *p) { Derived& d = *p; return d; }
+
+// If the target is a global, assume it's not safe to move from.
+static Derived global_d;
+Derived ok_global1() { return global_d; }
+Base ok_global2() { return global_d; }
+ConstructFromDerived ok_global3() { return global_d; }
+ConstructFromBase ok_global4() { return global_d; }
+ConvertFromDerived ok_global5() { return global_d; }
+ConvertFromBase ok_global6() { return global_d; }
+
+// If the target's copy constructor is trivial, assume the programmer doesn't care.
+TrivialDerived ok_trivial1(TrivialDerived d) { return d; }
+TrivialBase ok_trivial2(TrivialDerived d) { return d; }
+ConstructFromTrivialDerived ok_trivial3(TrivialDerived d) { return d; }
+ConstructFromTrivialBase ok_trivial4(TrivialDerived d) { return d; }
+ConvertFromTrivialDerived ok_trivial5(TrivialDerived d) { return d; }
+ConvertFromTrivialBase ok_trivial6(TrivialDerived d) { return d; }
+
+// If the target is a parameter, do apply the diagnostic.
+Derived testParam1(Derived d) { return d; }
+Base testParam2(Derived d) {
+    return d;  // e6
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConstructFromDerived testParam3(Derived d) {
+    return d;  // e7-cxx11
+    // expected-warning@-1{{would have been copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying on older compilers}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConstructFromBase testParam4(Derived d) {
+    return d;  // e8
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConvertFromDerived testParam5(Derived d) {
+    return d;  // e9
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConvertFromBase testParam6(Derived d) {
+    return d;  // e10
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+
+// If the target is an rvalue reference parameter, do apply the diagnostic.
+Derived testRParam1(Derived&& d) {
+    return d;  // e11
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+Base testRParam2(Derived&& d) {
+    return d;  // e12
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConstructFromDerived testRParam3(Derived&& d) {
+    return d;  // e13
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConstructFromBase testRParam4(Derived&& d) {
+    return d;  // e14
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConvertFromDerived testRParam5(Derived&& d) {
+    return d;  // e15
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+ConvertFromBase testRParam6(Derived&& d) {
+    return d;  // e16
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)"
+}
+
+// But if the return type is a reference type, then moving would be wrong.
+Derived& testRetRef1(Derived&& d) { return d; }
+Base& testRetRef2(Derived&& d) { return d; }
+auto&& testRetRef3(Derived&& d) { return d; }
+decltype(auto) testRetRef4(Derived&& d) { return (d); }
+
+// As long as we're checking parentheses, make sure parentheses don't disable the warning.
+Base testParens1() {
+    Derived d;
+    return (d);  // e17
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:15}:"std::move(d)"
+}
+ConstructFromDerived testParens2() {
+    Derived d;
+    return (d);  // e18-cxx11
+    // expected-warning@-1{{would have been copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:15}:"std::move(d)"
+}
+
+
+// If the target is a catch-handler parameter, do apply the diagnostic.
+void throw_derived();
+Derived testEParam1() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e19
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+Base testEParam2() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e20
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+ConstructFromDerived testEParam3() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e21
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+ConstructFromBase testEParam4() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e22
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+ConvertFromDerived testEParam5() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e23
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+ConvertFromBase testEParam6() {
+    try { throw_derived(); } catch (Derived d) { return d; }  // e24
+    // expected-warning@-1{{will be copied despite being returned by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)"
+    __builtin_unreachable();
+}
+
+// If the exception variable is an lvalue reference, we cannot be sure
+// that we own it; it is extremely contrived, but possible, for this to
+// be a reference to an exception object that was thrown via
+// `std::rethrow_exception(xp)` in Thread A, and meanwhile somebody else
+// has got a copy of `xp` in Thread B, so that moving out of this object
+// in Thread A would be observable (and racy) with respect to Thread B.
+// Therefore assume it's not safe to move from.
+Derived ok_REParam1() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+Base ok_REParam2() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+ConstructFromDerived ok_REParam3() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+ConstructFromBase ok_REParam4() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+ConvertFromDerived ok_REParam5() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+ConvertFromBase ok_REParam6() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); }
+
+Derived ok_CEParam1() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+Base ok_CEParam2() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+ConstructFromDerived ok_CEParam3() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+ConstructFromBase ok_CEParam4() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+ConvertFromDerived ok_CEParam5() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+ConvertFromBase ok_CEParam6() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); }
+
+// If rvalue overload resolution would find a copy constructor anyway,
+// or if the copy constructor actually selected is trivial, then don't warn.
+struct TriviallyCopyable {};
+struct OnlyCopyable {
+    OnlyCopyable() = default;
+    OnlyCopyable(const OnlyCopyable&) {}
+};
+
+TriviallyCopyable ok_copy1() { TriviallyCopyable c; return c; }
+OnlyCopyable ok_copy2() { OnlyCopyable c; return c; }
+TriviallyCopyable ok_copyparam1(TriviallyCopyable c) { return c; }
+OnlyCopyable ok_copyparam2(OnlyCopyable c) { return c; }
+
+void test_throw1(Derived&& d) {
+    throw d;  // e25
+    // expected-warning@-1{{will be copied despite being thrown by name}}
+    // expected-note@-2{{to avoid copying}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:12}:"std::move(d)"
+}
+
+void ok_throw1() { Derived d; throw d; }
+void ok_throw2(Derived d) { throw d; }
+void ok_throw3(Derived& d) { throw d; }
+void ok_throw4(Derived d) { throw std::move(d); }
+void ok_throw5(Derived& d) { throw std::move(d); }
+void ok_throw6(Derived& d) { throw static_cast<Derived&&>(d); }
+void ok_throw7(TriviallyCopyable d) { throw d; }
+void ok_throw8(OnlyCopyable d) { throw d; }