Skip to content

Commit

Permalink
[clang] P2266 implicit moves STL workaround
Browse files Browse the repository at this point in the history
This patch replaces the workaround for simpler implicit moves
implemented in D105518.

The Microsoft STL currently has some issues with P2266.

Where before, with -fms-compatibility, we would disable simpler
implicit moves globally, with this change, we disable it only
when the returned expression is in a context contained by
std namespace and is located within a system header.

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

Reviewed By: aaron.ballman, mibintc

Differential Revision: https://reviews.llvm.org/D105951
  • Loading branch information
mizvekov committed Jul 26, 2021
1 parent f84c70a commit 20555a1
Show file tree
Hide file tree
Showing 5 changed files with 200 additions and 79 deletions.
14 changes: 9 additions & 5 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -4785,20 +4785,24 @@ class Sema final {
bool isMoveEligible() const { return S != None; };
bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }
};
NamedReturnInfo getNamedReturnInfo(Expr *&E, bool ForceCXX2b = false);
enum class SimplerImplicitMoveMode { ForceOff, Normal, ForceOn };
NamedReturnInfo getNamedReturnInfo(
Expr *&E, SimplerImplicitMoveMode Mode = SimplerImplicitMoveMode::Normal);
NamedReturnInfo getNamedReturnInfo(const VarDecl *VD);
const VarDecl *getCopyElisionCandidate(NamedReturnInfo &Info,
QualType ReturnType);

ExprResult PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
const NamedReturnInfo &NRInfo,
Expr *Value);
ExprResult
PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
const NamedReturnInfo &NRInfo, Expr *Value,
bool SupressSimplerImplicitMoves = false);

StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
Scope *CurScope);
StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
StmtResult ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
NamedReturnInfo &NRInfo);
NamedReturnInfo &NRInfo,
bool SupressSimplerImplicitMoves);

StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
bool IsVolatile, unsigned NumOutputs,
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/Frontend/InitPreprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,7 @@ static void InitializeCPlusPlusFeatureTestMacros(const LangOptions &LangOpts,
}
// C++2b features.
if (LangOpts.CPlusPlus2b) {
if (!LangOpts.MSVCCompat)
Builder.defineMacro("__cpp_implicit_move", "202011L");
Builder.defineMacro("__cpp_implicit_move", "202011L");
Builder.defineMacro("__cpp_size_t_suffix", "202011L");
}
if (LangOpts.Char8)
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaCoroutine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
VarDecl *Promise = FSI->CoroutinePromise;
ExprResult PC;
if (E && (isa<InitListExpr>(E) || !E->getType()->isVoidType())) {
getNamedReturnInfo(E, /*ForceCXX2b=*/true);
getNamedReturnInfo(E, SimplerImplicitMoveMode::ForceOn);
PC = buildPromiseCall(*this, Promise, Loc, "return_value", E);
} else {
E = MakeFullDiscardedValueExpr(E).get();
Expand Down
61 changes: 39 additions & 22 deletions clang/lib/Sema/SemaStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3322,7 +3322,8 @@ Sema::ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope) {
/// \returns An aggregate which contains the Candidate and isMoveEligible
/// and isCopyElidable methods. If Candidate is non-null, it means
/// isMoveEligible() would be true under the most permissive language standard.
Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E, bool ForceCXX2b) {
Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E,
SimplerImplicitMoveMode Mode) {
if (!E)
return NamedReturnInfo();
// - in a return statement in a function [where] ...
Expand All @@ -3334,13 +3335,10 @@ Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E, bool ForceCXX2b) {
if (!VD)
return NamedReturnInfo();
NamedReturnInfo Res = getNamedReturnInfo(VD);
// FIXME: We supress simpler implicit move here (unless ForceCXX2b is true)
// in msvc compatibility mode just as a temporary work around,
// as the MSVC STL has issues with this change.
// We will come back later with a more targeted approach.
if (Res.Candidate && !E->isXValue() &&
(ForceCXX2b ||
(getLangOpts().CPlusPlus2b && !getLangOpts().MSVCCompat))) {
(Mode == SimplerImplicitMoveMode::ForceOn ||
(Mode != SimplerImplicitMoveMode::ForceOff &&
getLangOpts().CPlusPlus2b))) {
E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(),
CK_NoOp, E, nullptr, VK_XValue,
FPOptionsOverride());
Expand Down Expand Up @@ -3480,15 +3478,10 @@ VerifyInitializationSequenceCXX98(const Sema &S,
/// This routine implements C++20 [class.copy.elision]p3, 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.
ExprResult
Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
const NamedReturnInfo &NRInfo,
Expr *Value) {
// FIXME: We force P1825 implicit moves here in msvc compatibility mode
// because we are disabling simpler implicit moves as a temporary
// work around, as the MSVC STL has issues with this change.
// We will come back later with a more targeted approach.
if ((!getLangOpts().CPlusPlus2b || getLangOpts().MSVCCompat) &&
ExprResult Sema::PerformMoveOrCopyInitialization(
const InitializedEntity &Entity, const NamedReturnInfo &NRInfo, Expr *Value,
bool SupressSimplerImplicitMoves) {
if ((!getLangOpts().CPlusPlus2b || SupressSimplerImplicitMoves) &&
NRInfo.isMoveEligible()) {
ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
CK_NoOp, Value, VK_XValue, FPOptionsOverride());
Expand Down Expand Up @@ -3529,7 +3522,8 @@ static bool hasDeducedReturnType(FunctionDecl *FD) {
///
StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
Expr *RetValExp,
NamedReturnInfo &NRInfo) {
NamedReturnInfo &NRInfo,
bool SupressSimplerImplicitMoves) {
// If this is the first return we've seen, infer the return type.
// [expr.prim.lambda]p4 in C++11; block literals follow the same rules.
CapturingScopeInfo *CurCap = cast<CapturingScopeInfo>(getCurFunction());
Expand Down Expand Up @@ -3660,7 +3654,8 @@ StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
// the C version of which boils down to CheckSingleAssignmentConstraints.
InitializedEntity Entity = InitializedEntity::InitializeResult(
ReturnLoc, FnRetType, NRVOCandidate != nullptr);
ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRInfo, RetValExp);
ExprResult Res = PerformMoveOrCopyInitialization(
Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves);
if (Res.isInvalid()) {
// FIXME: Cleanup temporaries here, anyway?
return StmtError();
Expand Down Expand Up @@ -3870,15 +3865,37 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
return R;
}

static bool CheckSimplerImplicitMovesMSVCWorkaround(const Sema &S,
const Expr *E) {
if (!E || !S.getLangOpts().CPlusPlus2b || !S.getLangOpts().MSVCCompat)
return false;
const Decl *D = E->getReferencedDeclOfCallee();
if (!D || !S.SourceMgr.isInSystemHeader(D->getLocation()))
return false;
for (const DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent()) {
if (DC->isStdNamespace())
return true;
}
return false;
}

StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
// Check for unexpanded parameter packs.
if (RetValExp && DiagnoseUnexpandedParameterPack(RetValExp))
return StmtError();

NamedReturnInfo NRInfo = getNamedReturnInfo(RetValExp);
// HACK: We supress simpler implicit move here in msvc compatibility mode
// just as a temporary work around, as the MSVC STL has issues with
// this change.
bool SupressSimplerImplicitMoves =
CheckSimplerImplicitMovesMSVCWorkaround(*this, RetValExp);
NamedReturnInfo NRInfo = getNamedReturnInfo(
RetValExp, SupressSimplerImplicitMoves ? SimplerImplicitMoveMode::ForceOff
: SimplerImplicitMoveMode::Normal);

if (isa<CapturingScopeInfo>(getCurFunction()))
return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo);
return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo,
SupressSimplerImplicitMoves);

QualType FnRetType;
QualType RelatedRetType;
Expand Down Expand Up @@ -4069,8 +4086,8 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
// we have a non-void function with an expression, continue checking
InitializedEntity Entity = InitializedEntity::InitializeResult(
ReturnLoc, RetType, NRVOCandidate != nullptr);
ExprResult Res =
PerformMoveOrCopyInitialization(Entity, NRInfo, RetValExp);
ExprResult Res = PerformMoveOrCopyInitialization(
Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves);
if (Res.isInvalid()) {
// FIXME: Clean up temporaries here anyway?
return StmtError();
Expand Down
199 changes: 150 additions & 49 deletions clang/test/SemaCXX/cxx2b-p2266-disable-with-msvc-compat.cpp
Original file line number Diff line number Diff line change
@@ -1,52 +1,153 @@
// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions -verify=new %s
// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions -fms-compatibility -verify=old %s
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=old %s
// RUN: %clang_cc1 -std=c++2b -fsyntax-only -verify=cxx2b,new %s
// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fms-compatibility -verify=cxx2b,old %s
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=cxx20,old %s

// FIXME: This is a test for a temporary workaround where we disable simpler implicit moves
// when compiling with -fms-compatibility, because the MSVC STL does not compile.
// A better workaround is under discussion.
// The test cases here are just a copy from `CXX/class/class.init/class.copy.elision/p3.cpp`,
// so feel free to delete this file when the workaround is not needed anymore.

struct CopyOnly {
CopyOnly(); // new-note {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
// new-note@-1 {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
CopyOnly(CopyOnly &); // new-note {{candidate constructor not viable: expects an lvalue for 1st argument}}
// new-note@-1 {{candidate constructor not viable: expects an lvalue for 1st argument}}
};
struct MoveOnly {
MoveOnly();
MoveOnly(MoveOnly &&);
// in the STL when compiling with -fms-compatibility, because of issues with the
// implementation there.
// Feel free to delete this file when the workaround is not needed anymore.

#if __INCLUDE_LEVEL__ == 0

#if __cpluscplus > 202002L && __cpp_implicit_move < 202011L
#error "__cpp_implicit_move not defined correctly"
#endif

struct nocopy {
nocopy(nocopy &&);
};
MoveOnly &&rref();

MoveOnly &&test1(MoveOnly &&w) {
return w; // old-error {{cannot bind to lvalue of type}}
}

CopyOnly test2(bool b) {
static CopyOnly w1;
CopyOnly w2;
if (b) {
return w1;
} else {
return w2; // new-error {{no matching constructor for initialization}}
}
}

template <class T> T &&test3(T &&x) { return x; } // old-error {{cannot bind to lvalue of type}}
template MoveOnly &test3<MoveOnly &>(MoveOnly &);
template MoveOnly &&test3<MoveOnly>(MoveOnly &&); // old-note {{in instantiation of function template specialization}}

MoveOnly &&test4() {
MoveOnly &&x = rref();
return x; // old-error {{cannot bind to lvalue of type}}
}

void test5() try {
CopyOnly x;
throw x; // new-error {{no matching constructor for initialization}}
} catch (...) {
}

MoveOnly test6(MoveOnly x) { return x; }

int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }

namespace {
int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
namespace std {
int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }
} // namespace std
} // namespace foo

namespace std {

int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }

namespace {
int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &mt2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy mt3(nocopy x) { return x; }
} // namespace foo

} // namespace std

#include __FILE__

#define SYSTEM
#include __FILE__

#elif !defined(SYSTEM)

int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }

namespace {
int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }
namespace std {
int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }
} // namespace std
} // namespace foo

namespace std {

int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }

namespace {
int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &ut2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy ut3(nocopy x) { return x; }
} // namespace foo

} // namespace std

#else

#pragma GCC system_header

int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }

namespace {
int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }
namespace std {
int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // cxx2b-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }
} // namespace std
} // namespace foo

namespace std {

int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // new-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }

namespace {
int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // new-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }
} // namespace

namespace foo {
int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
int &st2(int &&x) { return x; } // new-error {{cannot bind to a temporary}}
nocopy st3(nocopy x) { return x; }
} // namespace foo

} // namespace std

#endif

0 comments on commit 20555a1

Please sign in to comment.