This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] workaround PR 28385 in __find_exactly_one_checked
ClosedPublic

Authored by CaseyCarter on Dec 9 2017, 9:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

CaseyCarter created this revision.Dec 9 2017, 9:18 AM
CaseyCarter added a comment.EditedDec 9 2017, 9:22 AM

Test?

Four of the existing variant tests fail without this change (Using the clang-6 dailies from apt.llvm.org and -std=c++2a):

Failing Tests (4):
    libc++ :: std/utilities/variant/variant.get/get_if_type.pass.cpp
    libc++ :: std/utilities/variant/variant.get/get_type.pass.cpp
    libc++ :: std/utilities/variant/variant.get/holds_alternative.pass.cpp
    libc++ :: std/utilities/variant/variant.variant/variant.mod/emplace_type_init_list_args.pass.cpp

  Expected Passes    : 36
  Unexpected Failures: 4

I think that's plenty of test coverage.

CaseyCarter edited the summary of this revision. (Show Details)Dec 9 2017, 11:26 AM
mclow.lists edited edge metadata.Dec 10 2017, 9:47 AM

These tests don't fail for me. (using a clang I built two days ago)

mclow.lists added inline comments.Dec 10 2017, 9:51 AM
include/tuple
1015 ↗(On Diff #126276)

We try not to have naked tests like this in the code.

If we have to, maybe:

#if defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER > 500 && _LIBCPP_STD_VER > 14

but I would rather define a new macro in <__config> and give it a meaningful name.

CaseyCarter added a comment.EditedDec 10 2017, 10:10 AM

These tests don't fail for me. (using a clang I built two days ago)

What about the repro for #35578? That repro and these four tests trigger the bug for me (no matching function for call to '__find_idx'...could not match "const bool" against "const bool") with clang version 6.0.0-svn320282-1~exp1 (trunk) freshly updated from apt.llvm.org on Ubuntu trusty running atop Windows Subsystem for Linux. (We've been shipping a workaround for 28385 in MSFT <variant> forever, and in range-v3 for even longer.) Both Godbolt and Wandbox agree.

These tests don't fail for me. (using a clang I built two days ago)

What about the repro for #35578? That repro and these four tests trigger the bug for me (no matching function for call to '__find_idx'...could not match "const bool" against "const bool") with clang version 6.0.0-svn320282-1~exp1 (trunk) freshly updated from apt.llvm.org on Ubuntu trusty running atop Windows Subsystem for Linux. (We've been shipping a workaround for 28385 in MSFT <variant> forever, and in range-v3 for even longer.) Both Godbolt and Wandbox agree.

What we've been trying to convey is, none of the current tests with current default params trigger the problem.
The difference being -std=c++1z. So some new tests are needed.

Ah - that was the factor I was missing.
The tests pass for me with -std=c++2a, but fail for std=c++17

Casey's original post said they fail with 2a, and I'm *still* not seeing that.

CaseyCarter added a comment.EditedDec 10 2017, 11:23 AM

Ah - that was the factor I was missing.
The tests pass for me with -std=c++2a, but fail for std=c++17

Casey's original post said they fail with 2a, and I'm *still* not seeing that.

They fail for me both with -std=c++2a and -std=c++17. (The wandbox & godbolt links above show the bug with the repro from 35578 and -std=c++2a.)

Hide the ugly version test in <__config>, define a slightly-more-meaningful macro _LIBCPP_WORKAROUND_CLANG_28385.

include/tuple
1015 ↗(On Diff #126276)

Better?

Make the workaround unconditional.

CaseyCarter marked 2 inline comments as done.Dec 11 2017, 11:41 AM

This unconditional workaround addresses Marshall's concerns about the naked version test.

lichray accepted this revision.Dec 11 2017, 9:17 PM
lichray added a subscriber: lichray.

Reproduced with

lit -sv --param=cxx_under_test="$HOME/bin/clang++" test/std/utilities/tuple/
lit: [...] note: Using available_features: ['libc++', 'verify-support', 'clang-6', 'modules-support', 'locale.en_US.UTF-8', 'diagnose-if-support', 'long_tests', 'fdelayed-template-parsing', '-faligned-allocation', 'locale.zh_CN.UTF-8', 'c++2a', 'locale.fr_CA.ISO8859-1', 'c++filesystem', 'c++experimental', 'clang', 'locale.fr_FR.UTF-8', 'locale.ru_RU.UTF-8', 'fsized-deallocation', 'freebsd11', 'fcoroutines-ts', 'locale.cs_CZ.ISO8859-2', 'clang-6.0', 'thread-safety']
This revision is now accepted and ready to land.Dec 11 2017, 9:17 PM

Better/simpler workaround from Zhihao.

This new-and-improved workaround - thanks to @lichray - is unintrusive enough that I can't imagine it being unnacceptable to anyone. I'm going to go ahead and check this in.