Page MenuHomePhabricator

[clang] P2266 implicit moves STL workaround
ClosedPublic

Authored by mizvekov on Jul 13 2021, 5:44 PM.

Details

Summary

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>

Diff Detail

Event Timeline

mizvekov created this revision.Jul 13 2021, 5:44 PM
mizvekov published this revision for review.Jul 13 2021, 6:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 6:39 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thank you for working on this! We're testing the patch out internally to see if it solves issues for us with some of the failures we're seeing.

clang/lib/Sema/SemaStmt.cpp
3315

This can return nullptr, so we should probably return false in that case; WDYT?

3318–3322

Can you use D->isInStdNamespace() instead?

mizvekov added inline comments.Jul 14 2021, 12:11 PM
clang/lib/Sema/SemaStmt.cpp
3315

Yes I missed that, you are absolutely right!

3318–3322

It doesn't look like isInStdNamespace is equivalent here, even though the name suggests otherwise.
This is a small helper, all it does is:

bool Decl::isInStdNamespace() const {
  const DeclContext *DC = getDeclContext();
  return DC && DC->isStdNamespace();
}

It helps you check isStdNamespace from a Decl, without having to through a DeclContext yourself.

Now if we really need this workaround to apply 'recursively' like this, that is a different question which I am not sure.

aaron.ballman added inline comments.Jul 14 2021, 12:31 PM
clang/lib/Sema/SemaStmt.cpp
3318–3322

Good point -- isInStdNamespace() only looks if the declaration is in the std namespace, not whether any declaration context containing the declaration is in std. I don't have an issue with the current approach, was mostly thinking if there were ways to simplify it.

mizvekov updated this revision to Diff 358702.EditedJul 14 2021, 12:46 PM
  • Small simplification to implementation, don't need to cast to NamespaceDecl.
  • Simplification to tests: Use some evil prep hackery to avoid needing multiple files.
  • Verify result of getReferencedDeclOfCallee is non-null.

No tests added for the last one, as I cannot think how this could actually happen in this instance.
That would suggest that maybe we should assert instead, but then it seems that if there were
such a case, it's most likely that we would want to return false anyway.
So the assert would be basically fishing for a test case. WDYT?

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3099,20): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
            return _Cleaned_link;
                   ^~~~~~~~~~~~~
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\memory(3566,5): note: 'unique_ptr' has been explicitly marked deleted here
    unique_ptr(const unique_ptr&) = delete;
    ^
In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3125,16): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
        return _Cleaned_link;
               ^~~~~~~~~~~~~
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\memory(3566,5): note: 'unique_ptr' has been explicitly marked deleted here
    unique_ptr(const unique_ptr&) = delete;

I used creduce with the same command line, looking for that error diagnostic with std:c++latest versus no diagnostic with std:c++20 ; this smaller test case, with a few edits, is the result

# 1 "test.cpp" 1
# 2 "test.cpp" 3
namespace std {
template <int a> struct b { static constexpr c = a; };
template <bool a> using d = b<a>;
using e = d<true>;
template <bool> using f = int;
template <bool, class, class> using g = int;
template <class...> constexpr am = e ::c;
template <class> constexpr au = false;
class h {
public:
  template <class i> h(i);
  template <class> using j = f<am<>>;
  template <class = j<g<au<int>, int, int>>> h(h &) = delete;
};
h k() {
  h l(new wchar_t);
  return l;
}
}
# 22 "test.cpp" 2

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

I think we might be in an awkward situation where some MSVC STL code needs the implicit cast in order to work, such as:

In file included from c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\string:11:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\xstring(4714,12): error: non-const lvalue reference to type 'basic_istream<...>' cannot bind to a temporary of type 'basic_istream<...>'
    return _Istr;
           ^~~~~

This issue was fixed with the previous workaround. But other MSVC STL code fails without the implicit cast, such as:

In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3099,20): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
            return _Cleaned_link;
                   ^~~~~~~~~~~~~

(At least, Melanie's reduced example fails with this patch, but if you remove the system header or namespace std bits from the test, then it passes.)

mizvekov updated this revision to Diff 359145.Jul 15 2021, 3:14 PM

Patch update:

  • Fixes issue where the workaround would completely supress implicit moves.
  • Improve tests to cover this issue.
  • Stop applying the workaround to throw expressions, as we have not observed any problems there.

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

I used creduce with the same command line, looking for that error diagnostic with std:c++latest versus no diagnostic with std:c++20 ; this smaller test case, with a few edits, is the result

Thanks for testing!

I think we might be in an awkward situation where some MSVC STL code needs the implicit cast in order to work, such as:
....
(At least, Melanie's reduced example fails with this patch, but if you remove the system header or namespace std bits from the test, then it passes.)

No, that was totally my fault! :-)

I did not intend for the workaround to completely disable implicit moves, just that it would fall back to the C++20 behavior.

The problem is that the code path for this is split around multiple places, and while suppressing simpler implicit moves here, I should have undone the suppressing of the C++20 implicit move code path elsewhere!

Sigh, I forgot it was there because I was working on another patch that completely removed that second code path...
Sorry for that!

While we are still reviewing this and it's probably going to take longer, I went ahead made a DR for fixing the same issue in main: https://reviews.llvm.org/D106303

FYI, we merged https://github.com/microsoft/STL/pull/2025 fixing the 2 affected return _Istr; occurrences in <string> and <xstring> for VS 2022 17.0 Preview 4. If you find any other affected return statements, please let us know ASAP (as the VS release process locks down the release branch far in advance).

FYI, we merged https://github.com/microsoft/STL/pull/2025 fixing the 2 affected return _Istr; occurrences in <string> and <xstring> for VS 2022 17.0 Preview 4. If you find any other affected return statements, please let us know ASAP (as the VS release process locks down the release branch far in advance).

Thanks!

We could relax the workaround even more, and apply them just to those affected member functions.
That would help finding more issues, if there are any.
But at the same time we are super close to creating the release branch...

mizvekov added a comment.EditedJul 25 2021, 2:59 PM

If you think landing this in 13 after the release branch is created will be complicated, then please consider this message a gentle Ping :)

Frankly, I am satisfied with the mechanics of this workaround as is.

  • After another look, I think doing something more targeted will be unduly convoluted, as we would want to target particular function overloads, one which is an operator>>.
  • The STL will still be tested upstream, with "-fno-ms-compatibility".
mibintc accepted this revision.Jul 26 2021, 10:00 AM

I tested this patch downstream on Windows with Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29334 for x64

I used c++20, x++latest default and c++17 with the stl header files (standalone include files--not usage) and didn't find any problems.

c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29333/

This revision is now accepted and ready to land.Jul 26 2021, 10:00 AM
aaron.ballman accepted this revision.Jul 26 2021, 12:17 PM

LGTM, thank you!

This revision was landed with ongoing or failed builds.Jul 26 2021, 1:21 PM
This revision was automatically updated to reflect the committed changes.