Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LLVM #2976

Merged
merged 81 commits into from
Oct 10, 2022
Merged

Update LLVM #2976

merged 81 commits into from
Oct 10, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jul 31, 2022

Updates the reference for the STL's llvm-project submodule. Doing so necessitates changes to:

  • basic_string::resize_and_overwrite to enable a libc++ test to pass
  • pointer_traits::pointer_to so "fancy pointers" work with constexpr basic_string (LWG-3454)
  • Additions and removals to the expected_results.txt and skipped_tests.txt files that control the external and internal libc++ test runners
  • Remove the workaround for LLVM-53957 which has been fixed
  • Sync P0088R3_variant, P0220R1_any, and P220R1_optional tests with their upstream libc++ sources

Drive-by:

  • sync tests/std/tests/P0088R3_variant/env.lst with usual_17_matrix.lst

Dual of MSVC-PR-428240.

CaseyCarter and others added 10 commits July 31, 2022 14:19
by passing non-`const` lvalues to the user's operation as specified.

Drive-by: avoid sign/conversion warnings in the debug checks, and explicitly convert the op result to `size_type` in the call to `_Eos`.
don't define `_SILENCE_ALL_CXX23_DEPRECATION_WARNINGS` and `_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING`
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Jul 31, 2022
@CaseyCarter CaseyCarter added the test Related to test code label Jul 31, 2022
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've investigated a few count of failed cases.
It seems that some case should have passed according to godbolt. I don't know what's going wrong.

tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
@fsb4000
Copy link
Contributor Author

fsb4000 commented Aug 1, 2022

What do we do when libc++ tests fail/pass on ARM but pass/fail on AMD64? Do we mark them SKIPPED?

And yes, I have read P0088R3_variant, P0220R1_any, P0220R1_optional tests and I will update them later.

@StephanTLavavej
Copy link
Member

What do we do when libc++ tests fail/pass on ARM but pass/fail on AMD64? Do we mark them SKIPPED?

Yeah - currently we don't have a way to do per-architecture FAILs.

@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in Code Reviews Aug 1, 2022
@frederick-vs-ja
Copy link
Contributor

This test fails for 32-bit platforms because of truncation from (unsigned) long long to 32-bit size_t. Perhaps we can and should fix the libc++ test by using static_cast.

  • std/numerics/rand/rand.dist/rand.dist.samp/rand.dist.samp.discrete/eval.pass.cpp

This test seemly fails for ARM/ARM64 platforms because MSVC STL considers tinyness_before to be always false even for ARM, but libc++ considers it to be true for floating-point types on ARM. I don't know which is right.

  • std/language.support/support.limits/limits/numeric.limits.members/tinyness_before.pass.cpp

Currently we may have to skip tests with difference results on difference architectures.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Aug 2, 2022

I created https://reviews.llvm.org/D130963

Copy link
Contributor

@cpplearner cpplearner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a non-exhaustive analysis.

tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter marked this pull request as ready for review October 6, 2022 10:56
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 6, 2022 10:56
@CaseyCarter

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

1 similar comment
@azure-pipelines

This comment was marked as outdated.

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
…amp/rand.dist.samp.discrete/eval.pass.cpp

frederick-vs-ja noticed that microsoft/STL#2976 (comment)
while we are working on updating LLVM submodule for MS STL:

    [...]\std\numerics\rand\rand.dist\rand.dist.samp\rand.dist.samp.discrete\eval.pass.cpp(33): error C2220: the following warning is treated as an error
    [...]\std\numerics\rand\rand.dist\rand.dist.samp\rand.dist.samp.discrete\eval.pass.cpp(287): note: see reference to function template instantiation 'void tests<__int64>(void)' being compiled
    [...]\std\numerics\rand\rand.dist\rand.dist.samp\rand.dist.samp.discrete\eval.pass.cpp(33): warning C4244: 'argument': conversion from '__int64' to 'const unsigned int', possible loss of data

Differential Revision: https://reviews.llvm.org/D130963

(cherry picked from commit db0ac307c9df26d26a629552aec0a78f1b492dfd)
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
…mory>` in tests

Found by @cpplearner (microsoft/STL#2976 (comment))

Differential Revision: https://reviews.llvm.org/D130997

(cherry picked from commit 495519e5f8232d144ed26e9c18dbcbac6a5f25eb)
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks very reasonable

@@ -32,7 +32,7 @@ PM_CL="/BE /c /EHsc /MD /std:c++latest /permissive-"
PM_CL="/BE /c /EHsc /MDd /std:c++17 /permissive-"
PM_CL="/BE /c /EHsc /MT /std:c++20 /permissive-"
PM_CL="/BE /c /EHsc /MTd /std:c++latest /permissive-"
PM_COMPILER="clang-cl" PM_CL="-fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++20 /permissive-"
PM_COMPILER="clang-cl" PM_CL="-fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++latest /permissive-"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does c++latest not imply /permissive- for clang?

Copy link
Member

@CaseyCarter CaseyCarter Oct 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK /permissive- is meaningless for clang and cl /BE. I'm only touching this file to sync it back up with usual_17_matrix like the comment says, I don't intend to make any other changes.

That said, we need to make a pass over the env lists one of these days to remove /permissive and /permissive- where they are the defaults and/or meaningless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang understands /permissive and /permissive- since clang 13: llvm/llvm-project@c70b0e8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what if anything /Zc:twoPhase- means to clang-cl, since it doesn't accept my test case:

namespace N {
    struct S {};
}

template <class T>
constexpr bool f(T) {
    return true;
}

template <class T>
constexpr bool g(T x) {
    return f(x);
}

constexpr bool f(N::S) {
    return false;
}

static_assert(!g(N::S{}), "");

I'm unable to determine if /std:c++20 and /std:c++latest imply /permissive-.

@strega-nil-ms
Copy link
Contributor

@CaseyCarter should this now be ready to merge, given that tests pass?

@CaseyCarter
Copy link
Member

CaseyCarter commented Oct 8, 2022

@CaseyCarter should this now be ready to merge, given that tests pass?

No, I'm still doing internal testing. (There are a couple classes of test cases that the GitHub test runner knows how to handle which the internal libcxx test runner does not. They need to be SKIPPED manually and aren't always easily detectible.) I'll move this to Final Review when it's green internally and externally.

* `.compile.pass.cpp` tests with no `main`
* `.sh.cpp` tests
* `.verify.cpp` tests
all of which break only in the internal test runner.
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Oct 10, 2022
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 10, 2022
@CaseyCarter CaseyCarter merged commit 1fe4d43 into microsoft:main Oct 10, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 10, 2022
@CaseyCarter CaseyCarter removed their assignment Oct 10, 2022
@CaseyCarter
Copy link
Member

Thanks so much for picking this up and finishing it off after I got busy, @fsb4000! Also thanks to everyone who helped analyze failures and classify issues.

@fsb4000 fsb4000 deleted the llvm_casey branch October 11, 2022 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants