Bug 217425 – REGRESSION(r268115) [GTK] Build failures with GCC 7 (Ubuntu 18.04) and GCC 8 (Debian Buster)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Lauro Moura]

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

[Alberto Garcia]

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.

[Lauro Moura]

Created attachment 410744 [details]
Especulative fix for GCC 8 and older

[Konstantin Tokarev]

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++

[Konstantin Tokarev]

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.

[Lauro Moura]

(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

[Lauro Moura]

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?

[Lauro Moura]

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.

[Lauro Moura]

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.

[Konstantin Tokarev]

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

[Carlos Alberto Lopez Perez]

(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?

[Lauro Moura]

(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)

[Carlos Alberto Lopez Perez]

(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.

[Lauro Moura]

Created attachment 411801 [details]
Patch with only compile check

[Carlos Alberto Lopez Perez]

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
)

[Lauro Moura]

Created attachment 411827 [details]
Patch for landing

Updated with APPEND. Mac wk1 failure is a flaky timeout failure.

[Lauro Moura]

Committed r268708: <https://trac.webkit.org/changeset/268708>