Skip to content

Conversation

@kmilos
Copy link
Collaborator

@kmilos kmilos commented Jan 8, 2025

No description provided.

@kmilos kmilos marked this pull request as draft January 8, 2025 13:31
@kmilos kmilos added the CMake Configuration issues related with CMake label Jan 8, 2025
@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

has this been tested?

I wouldn't expect it to be needed.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

has this been tested?

Not yet, hence the draft still...

I wouldn't expect it to be needed.

It is no different to other dependencies for static linkage?

I.e. looks like we've already also missed it in the private link interface for the still failing fuzz test... @kevinbackhouse

@kevinbackhouse
Copy link
Collaborator

Hmm. I was hoping that the CIFuzz would start working by itself after #2769 was merged. I can see from the build log that it is installing the libfmt-dev package now, so I'm not sure why it's not working. The build script that OSS-Fuzz uses is a fairly basic use of our cmake system: https://github.com/google/oss-fuzz/blob/master/projects/exiv2/build.sh. Can either of you see what might be wrong with it?

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

Not it. Looks like Ubuntu doesn't ship static libfmt.a in libfmt-dev...?

Edit: apart from MSYS2, looks like no distro is shipping the static lib (checked only major ones like Arch, Debian, Fedora)? In that case, this PR makes less sense indeed...

@kevinbackhouse
Copy link
Collaborator

Not it. Looks like Ubuntu doesn't ship static libfmt.a in libfmt-dev...?

Oh. So you think it's because of the -DBUILD_SHARED_LIBS=OFF? But we have other tests that do that.

@kevinbackhouse
Copy link
Collaborator

OSS-Fuzz is using Ubuntu 20.04, so maybe that's part of the issue.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

So you think it's because of the -DBUILD_SHARED_LIBS=OFF? But we have other tests that do that.

True that...

OSS-Fuzz is using Ubuntu 20.04, so maybe that's part of the issue.

Very likely, it ships an ancient version 6.1.2...

@kevinbackhouse
Copy link
Collaborator

Is there a cmake flag that will tell OSS-Fuzz to build without fmt? That might be the simplest solution. It's already building without the libinih library due to a similar issue.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

Not optional - it's either libfmt (we didn't check the minimum version? @neheb) or C++20 features in more recent libstdc++/libc++, but not without either option.

Might be time to bump requirements on main...

@kevinbackhouse
Copy link
Collaborator

Is there a flag to build with C++20? I could try that: I expect they offer that latest compiler versions on OSS-Fuzz even though they're on an old version of Ubuntu.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

Is there a flag to build with C++20? I could try that: I expect they offer that latest compiler versions on OSS-Fuzz even though they're on an old version of Ubuntu.

Looks like we might be in luck, it is indeed Clang 18... You can use the same flag we use for Cygwin builds (no libfmt there):

-DCMAKE_CXX_STANDARD=20 \

@kmilos kmilos force-pushed the kmilos/cmake_export_fmt branch from 14b05f0 to cbf4157 Compare January 9, 2025 12:58
@kmilos kmilos marked this pull request as ready for review January 9, 2025 13:00
@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

Is libfmt 6 not working?

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 9, 2025

Apparently not linking for some reason w/ -DBUILD_SHARED_LIBS=OFF. I don't think that config is covered anywhere else in CI w/ that particular libfmt version...

@kevinbackhouse
Copy link
Collaborator

The C++20 idea didn't work: kevinbackhouse/oss-fuzz#12

But I just tried building in a ubuntu:focal docker container and it works fine (with or without the C++20 flag). I'm building with clang-18. So it must be something else about the OSS-Fuzz environment.

@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

Apparently not linking for some reason w/ -DBUILD_SHARED_LIBS=OFF. I don't think that config is covered anywhere else in CI w/ that particular libfmt version...

tested ubuntu 20.04 with libfmt and clang++-10. works fine. exiv2 is not using special functionality in fmt. Just the std::format compatible stuff.

@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

The C++20 idea didn't work: kevinbackhouse/oss-fuzz#12

But I just tried building in a ubuntu:focal docker container and it works fine (with or without the C++20 flag). I'm building with clang-18. So it must be something else about the OSS-Fuzz environment.

/src/exiv2/src/pgfimage.cpp:32:23: error: no member named 'byteswap' in namespace 'std'

That...is a bug that needs fixing.

@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

@kevinbackhouse does this addition in src/pgfimage.cpp work?

#ifdef __cpp_lib_endian
#include <bit>
#endif

@kevinbackhouse
Copy link
Collaborator

@neheb: that std:byteswap error is a new thing that happened when I tried C++-23 rather than C++-20. I can't reproduce it on my own computer. It only happens in the OSS-Fuzz environment.

@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

I'll test in #2645 to see if CI can reproduce. It's most likely a missing bit header.

@kevinbackhouse
Copy link
Collaborator

@kevinbackhouse does this addition in src/pgfimage.cpp work?

#ifdef __cpp_lib_endian
#include <bit>
#endif

Yes, this solves the std::byteswap error. Unfortunately I'm now getting the same linker error with C++-23.

I guess it might be worth making this change anyway though?

@neheb
Copy link
Collaborator

neheb commented Jan 9, 2025

OK soo:

Ubuntu-20.04 does not come with the format header, so the check fails and fmt gets used. It uses the libc++ from gcc9. 13 introduces format.

libfmt gets found and used. But LD errors with

/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25

I think I've seen this but don't remember where.

Since oss-fuzz uses clang-18, would it be possible to also use lld-18?

edit: oooohhh that's exactly how I saw it. clang compiler with gcc's libc++ when I was testing meson CI.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 10, 2025

that's exactly how I saw it. clang compiler with gcc's libc++ when I was testing meson CI

There you install explicitly LLVM's libc++:

sudo apt install -y libc++abi-${{matrix.cxx}}-dev libc++-${{matrix.cxx}}-dev

Maybe that's the missing piece?

@neheb
Copy link
Collaborator

neheb commented Jan 10, 2025

No, that's unrelated.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 10, 2025

Also found this chestnut:

The implementation was complete since LLVM 14, but the feature-test macro was not set until LLVM 19

@kmilos kmilos force-pushed the kmilos/cmake_export_fmt branch from cbf4157 to 4dae7f2 Compare January 10, 2025 14:29
@neheb
Copy link
Collaborator

neheb commented Jan 10, 2025

Unfortunate. Looks like the check needs to be adjusted.

@neheb
Copy link
Collaborator

neheb commented Jan 10, 2025

anyway, CIFuzz failure. Some research led me to:

google/oss-fuzz#11698
https://gitlab.com/gnutls/gnutls/-/issues/1552

theory: libfmt is static, while everything else is shared, causing the error.
possible solution: build only static.

But then again I don't think that means cmake will use static versions of its dependencies.

I wonder if the issue is double linking.

Anyway, lld will definitely work.

@kevinbackhouse
Copy link
Collaborator

I tried modifying the build to use lld, but that doesn't work either: https://github.com/kevinbackhouse/oss-fuzz/actions/runs/12718238794/job/35456292416?pr=12

Do you think it's worth trying to build with meson rather than cmake, since you've already got that working on Ubuntu 20.04? Is there a way to build the fuzz build target with meson?

@neheb
Copy link
Collaborator

neheb commented Jan 11, 2025

I tried modifying the build to use lld, but that doesn't work either: https://github.com/kevinbackhouse/oss-fuzz/actions/runs/12718238794/job/35456292416?pr=12

Do you think it's worth trying to build with meson rather than cmake, since you've already got that working on Ubuntu 20.04? Is there a way to build the fuzz build target with meson?

That error looks like missing -lfmt in the linker flags for exiv2_int

Tests are generally speaking unimplemented in meson. I have a WIP commit that doesn’t fully work. Problem is the tests require making modifications to the source directory, not the build one from what I remember.

Fuzz target also needs implementing.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 11, 2025

That error looks like missing -lfmt in the linker flags for exiv2_int

One would've thought this should do it:

target_link_libraries(exiv2lib_int PRIVATE fmt::fmt)

@neheb
Copy link
Collaborator

neheb commented Jan 11, 2025

CMAKE_VERBOSE_MAKEFILE would confirm that.

if I had to guess it’s some fmt cmake bug.

It would be nice to convert fully to meson since it works properly, but extra code is needed to get the tests and fuzzing working.

edit: I wonder if that lld on that run is lld-18 or older.

@kevinbackhouse
Copy link
Collaborator

I added VERBOSE=1 to the make command line: https://github.com/kevinbackhouse/oss-fuzz/actions/runs/12728906903/job/35480066657?pr=12

@kevinbackhouse
Copy link
Collaborator

I've started a discussion on the OSS-Fuzz repo to ask for help: google/oss-fuzz#12934
Other than that, the only thing I can think of is to create a minimized version of the problem. It'll be a slow process: I'll need to create a fork of Exiv2 and then keep removing files and directories from it until I've got a much smaller project that triggers the same error.

@kevinbackhouse
Copy link
Collaborator

Meanwhile, we'll have to ignore the CIFuzz failures. Would you like to merge this PR?

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 13, 2025

Would you like to merge this PR?

I still think so...

@kmilos kmilos merged commit dd43045 into main Jan 13, 2025
63 of 64 checks passed
@mergify mergify bot deleted the kmilos/cmake_export_fmt branch January 13, 2025 17:41
@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

OK I just accidentally hit this in a different project. My error was having

-stdlib=libc++

in CXXFLAGS.

edit: If OSS-Fuzz mandates llvm libc++, then fmt must absolutely be built as a subproject.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 14, 2025

Ah, so that's maybe why I've seen a few projects vendor libmft...

In any case, I guess the options are:

  1. While waiting for OSS-Fuzz to hopefully bump to Clang 19, we fudge the detection a bit so c++20 std::format can be used
  2. Wait for OSS-Fuzz to upgrade to Ubuntu 22.04 where libfmt.so is shipped
  3. Build our own libmt for OSS-Fuzz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake Configuration issues related with CMake

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants