[Bug] Build failure on Fedora 44 (GCC 16)

[cphlipot]

Git commit

$ git rev-parse HEAD
3d6064b

Operating System & Version

Fedora 44

GGML backends

Vulkan

Command-line arguments used

N/A (project did not build)

Steps to reproduce

attempt build on Fedora 44 using instructions in build.md

mkdir build && cd build
cmake .. -DSD_VULKAN=ON
cmake --build . --config Release

What you expected to happen

project gets built successfully.

What actually happened

build failed

Logs / error messages / stack trace

error: no match for 'operator=' (operand types are 'std::string' and 'std::u8string')
  267 | lora_entry.name = p.stem().u8string();

Additional context / environment details

Fedora 44 Ships with GCC 16. GCC 16 will compile projects with C++20 by default unless the project overides the behavior which stable-diffusion.cpp does not.

https://gcc.gnu.org/gcc-16/changes.html#cxx

C++20 by default: GCC 16 changes the default language version for C++ compilation from -std=gnu++17 to -std=gnu++20.

in C++20 it does not allow direct assignment from std::u8string to std::string, which stable diffusion cpp currently does and results in compiler errors.

potential fixes include:

1. use std::string instead of u8string
2. convert project to use u8string instead of std::string
3. compile with --std=c++17 to get old compiler behavior with the newer GCC so we don't need to change the source code.

see also https://stackoverflow.com/questions/56833000/c20-with-u8-char8-t-and-stdstring

[wbruna]

unless the project overides the behavior which stable-diffusion.cpp does not.

Actually, we do:

https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/CMakeLists.txt#L256
https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/examples/cli/CMakeLists.txt#L20
https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/examples/server/CMakeLists.txt#L104

target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17)

And if I change that to C++20, I can reproduce that exact failure:

./examples/server/runtime.cpp: In function ‘void refresh_lora_cache(ServerRuntime&)’:
./examples/server/runtime.cpp:267:53: error: no match for ‘operator=’ (operand types are ‘std::string’ {aka ‘std::__cxx11::basic_string<char>’} and ‘std::u8string’ {aka ‘std::__cxx11::basic_string<char8_t>’})
  267 |             lora_entry.name     = p.stem().u8string();
      |    

so it's definitely active for our build targets.

[silvertuanzi]

Only set target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17) isn't enough.

In my archlinux I use a mix compiler:
CC: gcc-16
CXX: g++-16
CUDA: nvcc13.2 with host g++-15

Build will failed with default cmake .. -G Ninja -DSD_CUDA=ON
But cmake .. -G Ninja -DSD_CUDA=ON -DCMAKE_CXX_STANDARD=17 build successfully.

I use the same commit 3d6064b

[cphlipot]

target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17)

@wbruna Thanks for pointing to that. Based on my research and testing it looks like requesting a C++ feature set via target_compile_features() only guarantees a MINIMUM C++ version, not an EXACT C++ version. Since CMake assumes the GCC16 default C++20 already includes the features from C++17, it does not actually set -std=C++17 explicitly and just uses the default C++20 which causes issues with this project.

there is a bit more to read on that topic here: https://discourse.cmake.org/t/determining-the-compiler-default-c-standard/10389/7

The cxx_std_## features are for minimum standard levels. CXX_STANDARD will be honored even if it is lower than the compiler’s default, and an explicit -std= flag will be used to achieve it if necessary.

through the use of CMAKE_EXPORT_COMPILE_COMMANDS i can see the exact compiler options used to build runtime.cpp (the one with the errors)

mkdir build && cd build
cmake .. -DSD_VULKAN=ON
CMAKE_EXPORT_COMPILE_COMMANDS=1  cmake --build . --config Release
grep runtime.cpp compile_commands.json  | grep command

When i do this, i do not see -std=c++17 (or any other C++ version) being specified for this file. which is consistent with the hypothesis it is not being set because default is already c++20

/usr/lib64/ccache/c++ -DGGML_MAX_NAME=128 -DGGML_USE_CPU -DGGML_USE_VULKAN -DHAVE_INDEX_HTML -DSD_USE_WEBM -DSD_USE_WEBP -I/home/user/git/stable-diffusion.cpp/examples -I/home/user/git/stable-diffusion.cpp/examples/server/frontend/dist -I/home/user/git/stable-diffusion.cpp/. -I/home/user/git/stable-diffusion.cpp/src -I/home/user/git/stable-diffusion.cpp/include -I/home/user/git/stable-diffusion.cpp/thirdparty -I/home/user/git/stable-diffusion.cpp/ggml/src/../include -I/home/user/git/stable-diffusion.cpp/thirdparty/. -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebp/src -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebp -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebm -O3 -DNDEBUG -o CMakeFiles/sd-server.dir/runtime.cpp.o -c /home/user/git/stable-diffusion.cpp/examples/server/runtime.cpp

My recommendation is that we ensure -std=c++17 to be set explicitly both to resolve this compiler issue, and also to ensure that people with newer compilers don't accidentally introduce new C++ features to the codebase that would break those with older toolchains.

i think we can accomplish that by doing something like:

set_target_properties(${TARGET} PROPERTIES C_STANDARD 11 C_STANDARD_REQUIRED ON CXX_STANDARD 17 CXX_STANDARD_REQUIRED ON)

[jeeb]

Thanks for properly reporting this, I found this out a while ago since I have been running Fedora 44 since it was in beta, but as I did not see a clear way forward and was busy, I did not report it.

When looking into this, the problem for me was that:

• std::string and std::u8string were to my understanding always separate types
• f0f641a introduced the mis-match from the get-go in the LoraEntry structure, without explaining why this mis-match was needed. LoraEntry always had std::string there, but the code setting values to it was:

LoraEntry e;
e.name          = p.stem().u8string();
std::string rel = fs::relative(p, lora_dir).u8string();
std::replace(rel.begin(), rel.end(), '\\', '/');
e.path = rel;

So in other words, it was not clear what the intent was. Neither the commit message nor the PR say anything regarding it ( #1256 ) . If the intent was to have u8string s, then why was the structure defined with strings ? If there was a reason for the conversion by means of implicit casting (which C++ in general dislikes), then why was there no cast or mention as to why the types were mismatching between the structure and the input?

Of course, it doesn't help that GCC only started blocking this sort of code by default with version 16 (not sure if it even warned before about it), and clang 22 is still silent looking at a clang build I did. So catching this after writing it would have not been simple.

In other words, while forcing C++17 specifically would let it build, I think looking at what the intent of this code is would probably be a good idea as well?

[wbruna]

LoraEntry e;
e.name = p.stem().u8string();
std::string rel = fs::relative(p, lora_dir).u8string();
std::replace(rel.begin(), rel.end(), '\', '/');
e.path = rel;

So in other words, it was not clear what the intent was. Neither the commit message nor the PR say anything regarding it ( #1256 ) . If the intent was to have u8string s, then why was the structure defined with strings ? If there was a reason for the conversion by means of implicit casting (which C++ in general dislikes), then why was there no cast or mention as to why the types were mismatching between the structure and the input?

Since those strings come from filesystem names, and given the \ replacement, I believe the intent is avoiding encoding mismatches on Windows (Koboldcpp does something similar when loading models). If that's really the case, explicit conversions at this point should be fine. But we ideally need a way to at least get a warning on C++17, to avoid regressions.

[jeeb]

I do agree that compilers should warn about this stuff.

Also looking at previous IRC logs where I tried to wrap my head around whether the compilation error was valid or not, the following spec proposals were linked regarding char8_t and its integration into u8string usage:

• https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0482r6.html (OG proposal for char8_t, adopted for C++20)
• https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html (char8_t backward compatibility remediation)

[cphlipot]

I think we should still explictly set the C++ version which not only unblocks the build but also ensures going forward all devs are testing the build locally using the same C++17 version instead of whatever their local compiler defaults to. So even if we resolve the u8string issue, i think this would be good to keep.

I'm a a bit indifferent on the idea of warnings. personally i've found on other projects that warnings are often ignored unless also coupled with -werror to also turn them into compile errors, although I can't say for certain if that would apply to this project's devs too or not.

if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.

[cphlipot]

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html (char8_t backward compatibility remediation)

I found and skimmed this doc when doing original troubleshooting. I'm pretty sure the compiler errors we're seeing on Fedora44 fall under the "ill-formed" classification even with that remediation in place based on the Examples table there.

[jeeb]

Since those strings come from filesystem names, and given the \ replacement, I believe the intent is avoiding encoding mismatches on Windows (Koboldcpp does something similar when loading models). If that's really the case, explicit conversions at this point should be fine. But we ideally need a way to at least get a warning on C++17, to avoid regressions.

Alright, so specifically UTF-8 containing strings are wanted (I presume that is the whole wstring/string difference between Windows and non-Windows), then if the APIs these strings get pushed to only take in std::string the struct should contain that and std::string(s.begin(), s.end()) utilized when #if defined(__cpp_lib_char8_t) as per P1423R3 - or just always if the compiler is OK with it in C++17 mode. This will incur a cost due to the copying. Although I'm definitely not sure about where the C++ stdlib does character set conversions, if at all.

I think we should still explictly set the C++ version which not only unblocks the build but also ensures going forward all devs are testing the build locally using the same C++17 version instead of whatever their local compiler defaults to. So even if we resolve the u8string issue, i think this would be good to keep.

I'm a a bit indifferent on the idea of warnings. personally i've found on other projects that warnings are often ignored unless also coupled with -werror to also turn them into compile errors, although I can't say for certain if that would apply to this project's devs too or not.

if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.

-Wc++20-compat sounds like a good catch. Thanks, will test.

And yes, specific warnings should be made errors in the build system - many projects do that (when you limit to specific ones you are much less likely to get hit by a random change causing build failure with new toolchains).

[jeeb]

if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.

-Wc++20-compat sounds like a good catch. Thanks, will test.

Unfortunately at least clang 22.1.4 gives no additional warnings with this for examples/server/runtime.cpp (and looking at the specific warnings it adds the char8_t related stuff seems to be mostly about string literals as opposed to "live" strings: warning: type of UTF-8 string literal will change from array of const char to array of const char8_t in C++20 )

[wbruna]

I do agree that compilers should warn about this stuff.

Heh, they can't: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html#introduction mentions:

2. Change of return type for std::filesystem::path member functions u8string and generic_u8string.

So there's nothing to warn about on C++17. It was designed to be an annoying transition 🤦 Guess they got envious of Python 3 encoding mess...

[cphlipot]

@jeeb If the goal behind enabling a warning for this is to simply turn around and then expose it as an error. You may want to consider skipping the warning, and just enable the the compile feature for this directly so it will always error. This may also be the only option, as it sounds like there is no warning for this.

For example if we wanted to keep the c++ 17 base feature set and enable only this feature so u8 strings get checked at compile time without giving devs full access to C++20 features, we could do something like this:

if(MSVC)
    target_compile_options(${TARGET} PRIVATE /Zc:char8_t)
else()
    target_compile_options(${TARGET} PRIVATE -fchar8_t)
endif()