[libcxx-commits] [PATCH] D113161: [libc++] Implement P1989R2: range constructor for string_view

Wed Nov 24 07:57:09 PST 2021

jloser marked an inline comment as done.
jloser added a comment.

In D113161#3150604 <https://reviews.llvm.org/D113161#3150604>, @mstorsjo wrote:

> In D113161#3150454 <https://reviews.llvm.org/D113161#3150454>, @jloser wrote:
>
>> @ldionne why does the `clang-cl dynamic` build fail but `clang-cl static` build succeed? The dynamic build hard errors about use of `[[no_unique_address]]` unrelated to this change I'd argue (it exists on top-of-tree). Relevant logs are at https://buildkite.com/llvm-project/libcxx-ci/builds/6840.
>>
>> Note the warnings show up in both `clang-cl dynamic` and `clang-cl static` but only hard errors in the `dynamic` case.
>
> The error isn't about `[[no_unique_address]]`, the actual error is further up in the log:
>
>   FAILED: libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj 
>   "C:\Program Files\LLVM\bin\clang-cl.exe"  /nologo -TP -DNDEBUG -DUNICODE -D_ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH -D_ALLOW_MSC_VER_MISMATCH -D_CRTBLD -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\ws\w5\llvm-project\libcxx-ci\libcxx\src -D_LIBCPP_HAS_NO_INT128 /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw --target=x86_64-pc-windows-msvc /MD /Zi /O2 /Ob1 /DNDEBUG --target=x86_64-pc-windows-msvc -W4 -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-c++11-compat -Wno-undef -Wno-reserved-id-macro -Wno-gnu-include-next -Wno-gcc-compat -Wno-zero-as-null-pointer-constant -Wno-deprecated-dynamic-exception-spec -Wno-sign-conversion -Wno-old-style-cast -Wno-deprecated -Wno-shift-sign-overflow -Wno-double-promotion -Wno-error -EHsc /IC:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -std:c++latest /showIncludes /Folibcxx\src\CMakeFiles\cxx_shared.dir\filesystem\operations.cpp.obj /Fdlibcxx\src\CMakeFiles\cxx_shared.dir\ -c -- C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp
>   In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
>   In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:252:
>   In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\iterator:588:
>   C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\__iterator/counted_iterator.h(68,5): warning: unknown attribute 'no_unique_address' ignored [-Wunknown-attributes]
>     [[no_unique_address]] _Iter __current_ = _Iter();
>       ^~~~~~~~~~~~~~~~~
>   In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
>   In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:255:
>   In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string:532:
>   C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(764,6): error: definition with same mangled name '??$?9_WU?$char_traits at _W@__1 at std@@@__1 at std@@YA_NV?$basic_string_view at _WU?$char_traits at _W@__1 at std@@@01 at 0@Z' as another definition
>   bool operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
>        ^
>   C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(773,6): note: previous definition is here
>   bool operator!=(basic_string_view<_CharT, _Traits> __lhs,
>        ^
>   1 warning and 1 error generated.
>
> (That said, those warnings are super noisy, it'd be great to get rid of them - I've been meaning to take a look at it but haven't gotten to it.)

Thanks for spotting that, @mstorsjo! It does look indeed related to my fix for the atomic constraints caching issue on windows for `path`. I've just pushed a workaround for `string_view::operator!=` which may work - we'll see what CI thinks. :)

Note the comparison ops for `string_view` already have this "issue" with duplicate mangled names I think - it's just not getting hit from call sites in `libc++` in a dynamic library CI case before this change, right?

================
Comment at: libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp:13-16
+// This test fails due to atomic constraints caching on clang-cl. Specifically, the constraints of
+// std::ranges::range concept changes over the lifetime of this TU. Similar story with libc++ and MinGW.
+// XFAIL: target={{.+}}-w64-windows-gnu
+// XFAIL: msvc
----------------
mstorsjo wrote:
> jloser wrote:
> > mizvekov wrote:
> > > Quuxplusone wrote:
> > > > This sounds sketchy to me. Can you or @mizvekov explain further? (Explain in a thread here, that is. The code comment can be amended, if needed, after I get it :))
> > > > 
> > > > This test just includes a couple of libc++ headers and then tests some properties of `fs::path`. If this simple stuff doesn't work, then that smells like a libc++ bug (still), not something for the end-user to work around. In particular, this smells like we still have the same kind of issue as in `__lhs.compare(__rhs)`, but with some other member function, and in a codepath that gets compiled only on Windows... no?
> > > I don't think caching is any different between clang and clang-cl.
> > > 
> > > I think the initial issue, from what I read about it, has to do with concepts satisfaction caching and how clang does not implement any kind of invalidation of the cache.
> > > 
> > > This could be tested by passing frontend command line switch `-fno-concept-satisfaction-caching`, or if using the driver, `-Xclang -fno-concept-satisfaction-caching`.
> > > 
> > > But the difference in behavior between clang and clang-cl must be something else.
> > I'll precursor my comment with the fact that I don't have a great way to test the `clang-cl` or `MinGW` failing builds locally. It was failing for similar reasons with atomic constraints caching on CI. I think it's a similar issue with the `__lhs.compare(__rhs)` as you mentioned that is only a codepath for when `_LIBCPP_WIN32API` is defined.
> > 
> > I've reproduced the issue locally with `ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_WIN32API=1` to emulate a "windows only code path" that this error shows up. I'll try debugging it soon to figure out the problem.
> Also just for future reference, if you want to XFAIL something for windows overall (both clang-cl and mingw), it's enough with just `XFAIL: windows`, or `XFAIL: {{.+}}-windows{{.*}}` for regex matching the triple for both variants).
Good to know - thanks!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113161/new/

https://reviews.llvm.org/D113161