Ubuntu 18.04: gcc 7.5.0 `<filesystem>` is available as `<experimental/filesystem>` (`std::experimental::filesystem` namespace) and needs `-lstdc++fs` Debian Buster: gcc 8.3.0 Also would need `-lstdc++fs` Tools/WebKitTestRunner/CMakeFiles/WebKitTestRunner.dir/TestOptions.cpp.o:TestOptions.cpp:function WTR::parseTestHeader(std::filesystem::__cxx11::path, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&): error: undefined reference to 'std::filesystem::status(std::filesystem::__cxx11::path const&)' Tools/WebKitTestRunner/CMakeFiles/WebKitTestRunner.dir/TestOptions.cpp.o:TestOptions.cpp:function WTR::featureDefaultsFromTestHeaderForTest(WTR::TestCommand const&): error: undefined reference to 'std::filesystem::__cxx11::path::_M_split_cmpts()' collect2: error: ld returned 1 exit status
From the GCC 9 changelog: Using the types and functions in <filesystem> does not require linking with -lstdc++fs now. So you can check for that.
Created attachment 410744 [details] Especulative fix for GCC 8 and older
Comment on attachment 410744 [details] Especulative fix for GCC 8 and older View in context: https://bugs.webkit.org/attachment.cgi?id=410744&action=review > Source/cmake/WebKitCompilerFlags.cmake:140 > + if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS "9.0" AND NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang") This is not a correct condition, at very least it should use CMAKE_COMPILER_IS_GNUCXX to test for GCC. However, it also leaves Clang broken on Linux, where it uses libstdc++ from GCC by default[*]. Adding -lstdc++fs to compiler flags is also erroneous, because it is a linking flag. I think we need linking test here, if stdc++fs is accepted it should be added to global linker flags like it's done for SANITIZER_LINK_FLAGS. [*] While clang has an option for libc++, it needs to be installed separately, and produced binaries are not compatible with c++ libraries built against libc++
Probably the best approach would be to use check_cxx_source_compiles() with minimal <filesystem> usage. It should work like ATOMIC_TEST_SOURCE check in WebKitCompilerFlags.cmake and will be very similar in implementation.
(In reply to Konstantin Tokarev from comment #4) > Probably the best approach would be to use check_cxx_source_compiles() with > minimal <filesystem> usage. It should work like ATOMIC_TEST_SOURCE check in > WebKitCompilerFlags.cmake and will be very similar in implementation. Right, will check it. The original patch that introduced this issue was reverted in r268143. Btw, this filesystem issue with older gcc is still an issue within cmake: https://gitlab.kitware.com/cmake/cmake/-/issues/17834
Created attachment 411406 [details] Tentative patch with actual cmake test source Updated patch with sample program to test. Tested the patch against gcc-9/8/7 and clang-7. The original file (from LLVM) contains no copyright. Should it be added?
Comment on attachment 411406 [details] Tentative patch with actual cmake test source View in context: https://bugs.webkit.org/attachment.cgi?id=411406&action=review Some self comments. > Source/WTF/wtf/StdFilesystem.h:12 > +#elif HAVE(STD_EXPERIMENTAL_FILESYSTEM) Maybe USE(STD_EXPERTIMENTAL_FILESYSTEM) would have been better than HAVE? Also, given stdc++fs is added to WTR_Libraries only for GCC/CLANG, should this guard be here too? (Not sure how this would affect mac builds, especially for 10.14, which was the original target of this file) > Source/cmake/OptionsCommon.cmake:159 > + SET_AND_EXPOSE_TO_BUILD(HAVE_STD_EXPERIMENTAL_FILESYSTEM TRUE) Ditto. > Tools/WebKitTestRunner/CMakeLists.txt:32 > + ${WebKitTestRunner_LIBRARIES} I tried CMAKE_CXX_IMPLICIT_LINK_LIBRARIES but it seemed to be ignored. And adding to FLAGS caused it to come before the target file, still causing link errors.
Ah well. JSC bots (cross compiled) failed the check_cxx_source_runs check. I had to add it (instead of just compile) because in GCC 8 the <filesystem> test builds without stdc++fs, but segfaults at runtime.
If it's not possible to change test to avoid segfault at run time, it's better to change policy to link stdc++fs by default, and omit it only if linking fails
(In reply to Lauro Moura from comment #8) > Ah well. JSC bots (cross compiled) failed the check_cxx_source_runs check. > > I had to add it (instead of just compile) because in GCC 8 the <filesystem> > test builds without stdc++fs, but segfaults at runtime. This is a problem. Our build needs to be cross-compiler friendly. We build WebKit a lot for foreign architectures with Yocto and Buildroot We need to figure out of a way of doing this test without actually running the executable. Why this simple test compiles with GCC8 but WebKit doesn't?
(In reply to Carlos Alberto Lopez Perez from comment #10) > (In reply to Lauro Moura from comment #8) > > Ah well. JSC bots (cross compiled) failed the check_cxx_source_runs check. > > > > I had to add it (instead of just compile) because in GCC 8 the <filesystem> > > test builds without stdc++fs, but segfaults at runtime. > > This is a problem. Our build needs to be cross-compiler friendly. We build > WebKit a lot for foreign architectures with Yocto and Buildroot Yeah, this flew under my radar while testing the patch. > > We need to figure out of a way of doing this test without actually running > the executable. > > Why this simple test compiles with GCC8 but WebKit doesn't? Checking the bots errors again, I managed to reproduce WebKit's compile error with the snippet inside an ubuntu18 container. Meanwhile, in my Ubuntu 20 system, GCC8 compiled WebKit normally, segfaulting on std::filesystem::path destructor. (In reply to Konstantin Tokarev from comment #9) > If it's not possible to change test to avoid segfault at run time, it's > better to change policy to link stdc++fs by default, and omit it only if > linking fails The error in my previous paragraph led here, the same GCC8 error: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90050 It ended up being closed as invalid, with the solution for the reporter being to always link stdc++fs (at least with GCC8)
(In reply to Lauro Moura from comment #11) > > > > We need to figure out of a way of doing this test without actually running > > the executable. > > > > Why this simple test compiles with GCC8 but WebKit doesn't? > > Checking the bots errors again, I managed to reproduce WebKit's compile > error with the snippet inside an ubuntu18 container. > > Meanwhile, in my Ubuntu 20 system, GCC8 compiled WebKit normally, > segfaulting on std::filesystem::path destructor. > > (In reply to Konstantin Tokarev from comment #9) > > If it's not possible to change test to avoid segfault at run time, it's > > better to change policy to link stdc++fs by default, and omit it only if > > linking fails > > The error in my previous paragraph led here, the same GCC8 error: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90050 > > It ended up being closed as invalid, with the solution for the reporter > being to always link stdc++fs (at least with GCC8) Ok. That is likely caused because you are using a newer libstdc++ (the one that comes with GCC9) with an older compiler (GCC8). This comment on that bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90050#c4 suggest also this. That is a very corner case that maybe we can leave unfixed for the moment. I expect that the version of GCC and libstdc++ should match, at least will do in the 99.9% of the real use cases. I think that when one uses a different version of GCC than libstdc++ is actually for using a newer version of the compiler and not an older one. I wonder what happens with Clang on Ubuntu20?: does this test fail to build without -lstdc++fs or builds but segfaults? if clang doesn't segfault then that indicates the issue is only with that weird combination of gcc8 with libstdc++-9, so I think we can leave that unfixed.
Created attachment 411801 [details] Patch with only compile check
Comment on attachment 411801 [details] Patch with only compile check View in context: https://bugs.webkit.org/attachment.cgi?id=411801&action=review > Tools/WebKitTestRunner/CMakeLists.txt:33 > + set(WebKitTestRunner_LIBRARIES > + ${WebKitTestRunner_LIBRARIES} > + stdc++fs perhaps is maybe better to use here: list(APPEND WebKitTestRunner_LIBRARIES stdc++fs )
Created attachment 411827 [details] Patch for landing Updated with APPEND. Mac wk1 failure is a flaky timeout failure.
Committed r268708: <https://trac.webkit.org/changeset/268708>