Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the libjpeg and libjxl pc files #79

Closed
wants to merge 1 commit into from

Conversation

DDoS
Copy link

@DDoS DDoS commented Oct 9, 2024

Hi, I've been trying to use wasm-vips in a project, and came upon a couple of build errors when linking to my executable. I thought I'd share the fixes.

  • libjpeg: use the prefix correctly
  • libjxl: move lc++ to private libs to fix wasm-ld: error: --shared-memory is disallowed by hash.o because it was not compiled with 'atomics' or 'bulk-memory'

Build was done with --disable-bindings --disable-modules --enable-libvips-cpp.

@kleisauke
Copy link
Owner

Hi @DDoS,

  • libjpeg: use the prefix correctly

Fixed in commit 4dc513a. I'll discuss this patch upstream later.

  • libjxl: move lc++ to private libs to fix wasm-ld: error: --shared-memory is disallowed by hash.o because it was not compiled with 'atomics' or 'bulk-memory'

This doesn't seem correct. You'll need to ensure that your executable is compiled with -pthread too, see commit 396c85b for details. Additionally, you'll also need to use pkg-config --static --libs vips-cpp to ensure your executable is linked against aom (see e.g. the discussion in lovell/sharp#3522 (comment)).

@DDoS
Copy link
Author

DDoS commented Oct 10, 2024

Fixed in commit 4dc513a. I'll discuss this patch upstream later.

Nice!

This doesn't seem correct. [...]

With that change, aom is now in the list of libraries (thanks for mentioning the --static flag, I didn't realize CMake didn't handle that automatically), but the error persists. In fact I have to remove lc++ entirely to fix the link. I realize that my original fix just effectively removed the only public lc++, which just removed it from the link.
I have been compiling my project with -pthread. I only see the error when I enable jxl. I got the build options from here, but I removed --disable-jxl because I wanted that feature. Kind of wondering if that's why they disabled it.

EDIT: Emscripten is adding lc++-mt and lc++abi-mt to the link, and it looks like lc++ from pkgconfig is linking against emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++.a, which is not multi-threaded?

I also get these warnings, if you happen to know anything about these.

[build] wasm-ld: warning: function signature mismatch: __extendhfsf2
[build] >>> defined as (i32) -> f32 in /Users/sapon/Sources/wasm-vips/build/target/lib/libresvg.a(compiler_builtins-a41c940a04bdc92b.compiler_builtins.74069b6e81eca72d-cgu.06.rcgu.o)
[build] >>> defined as (f32) -> f32 in /Users/sapon/Sources/wasm-vips/build/target/lib/libresvg.a(compiler_builtins-a41c940a04bdc92b.compiler_builtins.74069b6e81eca72d-cgu.15.rcgu.o)
[build]
[build] wasm-ld: warning: function signature mismatch: __truncsfhf2
[build] >>> defined as (f32) -> i32 in /Users/sapon/Sources/wasm-vips/build/target/lib/libresvg.a(compiler_builtins-a41c940a04bdc92b.compiler_builtins.74069b6e81eca72d-cgu.05.rcgu.o)
[build] >>> defined as (f32) -> f32 in /Users/sapon/Sources/wasm-vips/build/target/lib/libresvg.a(compiler_builtins-a41c940a04bdc92b.compiler_builtins.74069b6e81eca72d-cgu.11.rcgu.o)

@DDoS
Copy link
Author

DDoS commented Oct 10, 2024

Also I think at this point this is better off as an issue than a PR, so I'm closing this.

@DDoS DDoS closed this Oct 10, 2024
DDoS pushed a commit to DDoS/wasm-vips that referenced this pull request Oct 10, 2024
@kleisauke
Copy link
Owner

Kind of wondering if that's why they disabled it.

Please see: lovell/sharp#2731 (comment).

EDIT: Emscripten is adding lc++-mt and lc++abi-mt to the link, and it looks like lc++ from pkgconfig is linking against emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++.a, which is not multi-threaded?

Are you compiling with the same Emscripten version as wasm-vips? Emscripten doesn't guarantee ABI stability between versions, so the versions need to be aligned for compatibility.

I also get these warnings, if you happen to know anything about these.

[build] wasm-ld: warning: function signature mismatch: __extendhfsf2
[...]
[build] wasm-ld: warning: function signature mismatch: __truncsfhf2
[...]

The signature mismatch warnings for __extendhfsf2 and __truncsfhf2 were resolved in commit rust-lang/compiler-builtins@6a80b90 and rust-lang/rust@57b164a, so it looks like you're using an outdated version of libresvg.a. You might need to clean up existing cache with:

$ rm -rf build/{cargo-cache,ccache,deps,emcache,target}

This ensures that all dependencies are rebuilt.

@DDoS
Copy link
Author

DDoS commented Oct 11, 2024

Are you compiling with the same Emscripten version as wasm-vips?

Looks like I was using a slightly newer version (3.1.68 instead of 3.1.64). I downgraded my project, but the error remains :/

it looks like you're using an outdated version of libresvg.a

In fact I don't need SVG support, so I'll just disable it.

@kleisauke
Copy link
Owner

I downgraded my project, but the error remains :/

Hmm, weird. Could you reproduce this issue even when --disable-jxl is passed? libheif also depends on -lc++, so it might be worth checking if this issue only occurs when libjxl was enabled.

wasm-vips/build.sh

Lines 481 to 483 in 7b3657e

# ... and the same for -lc++, see:
# https://github.com/emscripten-core/emscripten/issues/16680#issuecomment-1558015445
sed -i '/^Libs.private:/s/ -lc++//' $TARGET/lib/pkgconfig/libheif.pc

Note to self: hash.o might originate from:
https://github.com/emscripten-core/emscripten/blob/main/system/lib/libcxx/src/hash.cpp

@DDoS
Copy link
Author

DDoS commented Oct 16, 2024

Could you reproduce this issue even when --disable-jxl is passed?

Yes, once I added the --static flag to pkg-config. Turns out CMake has a problem handling pkg-config static libraries that was making my project link as shared instead. So the JXL thing was just a coincidence, it happened to be the only one declaring lc++ as public.

Note to self: hash.o might originate from:

That's where it's coming from. I have checked all the library archives generated by the build and haven't found anything else.

I'm pretty sure that the problem is there are multiple versions of libc++ in emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/, specifically libc++-mt*.a variants for multithreading (-pthread). But pkg-config only specifies lc++, which gets interpreted (by CMake?) as libc++.a, without multithreading. Emsdk also adds the correct libc++-mt.a libraries to the link, but that doesn't prevent the hash.o link error from the libc++.a. My fix has to been to remove lc++ from the CMake imported target libraries.

# Remove libc++ from pkg-config, because it conflicts with multithreaded
# libc++ from emscripten
get_target_property(vips_libraries PkgConfig::vips INTERFACE_LINK_LIBRARIES)
 list(FILTER vips_libraries EXCLUDE REGEX [[libc\+\+]])
set_target_properties(PkgConfig::vips PROPERTIES
    INTERFACE_LINK_LIBRARIES "${vips_libraries}")

@kleisauke
Copy link
Owner

Just a sanity check, are you also linking with -pthread? In Emscripten, this block should remap -lc++ to libc++-mt.a, but only when -pthread is specified as a link argument:
https://github.com/emscripten-core/emscripten/blob/3.1.69/tools/link.py#L2771-L2777

It may also be worth checking that you are not accidentally linking with the -no-pthread argument or the deprecated -sUSE_PTHREADS=0 setting.

@DDoS
Copy link
Author

DDoS commented Oct 17, 2024

Here's the full command line

~/Sources/Cadre/emsdk/upstream/emscripten/em++ -O2 -g -DNDEBUG --preload-file=~/Sources/Cadre/encre/test_data@/ -sSAFE_HEAP=1 -sASSERTIONS=2 --bind -sENVIRONMENT=web,worker -sALLOW_MEMORY_GROWTH=1 -sABORTING_MALLOC=0 -sMALLOC=mimalloc -sSTACK_SIZE=256KB -sALLOW_TABLE_GROWTH -sTEXTDECODER=2 -sFORCE_FILESYSTEM -sEXCEPTION_STACK_TRACES "-sPTHREAD_POOL_SIZE='Math.max(navigator.hardwareConcurrency,6)'" -pthread -fexceptions -msimd128 -sWASM_BIGINT js/CMakeFiles/js_encre.dir/src/js_encre.cpp.o -o js/js_encre.js -L~/Sources/wasm-vips/build/target/lib -Wl,--whole-archive  core/libencre.a  -Wl,--no-whole-archive  vcpkg_installed/wasm32-emscripten-pthread/lib/libglm.a  vcpkg_installed/wasm32-emscripten-pthread/lib/libqhullstatic_r.a  vcpkg_installed/wasm32-emscripten-pthread/lib/libqhullcpp.a  -lvips-cpp  -lvips  -lgio-2.0  -lgmodule-2.0  -lgobject-2.0  -lglib-2.0  -lffi  -lexpat  -limagequant  -lcgif  -lexif  -lspng  -lwebpmux  -lwebpdemux  -ltiff  -lwebp  -ljpeg  -lz  -lwebp  -ljpeg  -lz  -lheif  ~/Sources/Cadre/emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++.a  -laom  -lsharpyuv  -ljxl  -lhwy  -lbrotlienc  -lbrotlidec  -lbrotlicommon  -ljxl_cms  ~/Sources/Cadre/emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++.a  -laom  -lsharpyuv  -ljxl  -lhwy  -lbrotlienc  -lbrotlidec  -lbrotlicommon  -ljxl_cms  -llcms2  -ljxl_threads  -lm

libc++ is being passed as an absolute path, instead of lc++. Might be a CMake issue, since it's generating that command line.

@kleisauke
Copy link
Owner

Indeed, it seems to be linking against libc++.a instead of libc++-mt.a, which is strange.

I tried several things to reproduce this but without success. The only way I could trigger a similar --shared-memory is disallowed by ...-error was by making the following change:

--- a/build.sh
+++ b/build.sh
@@ -511,6 +511,7 @@ EOL
   stage "Compiling JS bindings"
   mkdir $DEPS/wasm-vips
   cd $DEPS/wasm-vips
+  CXXFLAGS=${CXXFLAGS/ -pthread/} \
   emcmake cmake $SOURCE_DIR -DCMAKE_BUILD_TYPE=Release -DCMAKE_RUNTIME_OUTPUT_DIRECTORY="$SOURCE_DIR/lib" \
     -DENVIRONMENT=${ENVIRONMENT//,/;} -DENABLE_MODULES=$MODULES -DENABLE_WASMFS=$WASM_FS
   make

Are you able to provide a complete, standalone code sample that allows someone else to reproduce this issue?

@DDoS
Copy link
Author

DDoS commented Oct 19, 2024

I stripped down my project to the bare essentials: repro.zip

Build steps:

  • clone https://github.com/emscripten-core/emsdk.git in the root directory (the script will do the install and activate later).
  • set WASM_VIPS_ROOT to the root of wasm-vips (for example export WASM_VIPS_ROOT=~/Sources/wasm-vips)
    • Use branch pkgconfig-fixes of https://github.com/DDoS/wasm-vips.git, which is v0.0.10 + Make pkg-config file of MozJPEG relocatable (#79)
  • run cmake --workflow --preset web
  • it should fail as discussed
  • search for UNCOMMENT TO FIX to find my fix, that should make the build succeed now

Thanks for the help with this!

@kleisauke
Copy link
Owner

Thanks for the standalone reproduction! I'm still investigating the root cause (which I suspect is a CMake quirk), but this patch resolves the issue for me:

Details
--- a/CMakePresets.json
+++ b/CMakePresets.json
@@ -13,7 +13,7 @@
             "cacheVariables": {
                 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/cmake/FindEmsdkToolchain.cmake",
                 "CMAKE_EXPORT_COMPILE_COMMANDS": "ON",
-                "PKG_CONFIG_ARGN": "--define-variable=prefix=$env{WASM_VIPS_ROOT}/build/target;--static",
+                "PKG_CONFIG_ARGN": "--define-variable=prefix=$env{WASM_VIPS_ROOT}/build/target",
                 "CMAKE_BUILD_TYPE": "RelWithDebInfo"
             },
             "environment": {
--- a/core/CMakeLists.txt
+++ b/core/CMakeLists.txt
@@ -21,17 +21,8 @@ target_sources(encre PRIVATE
 )
 
 find_package(PkgConfig REQUIRED)
-pkg_search_module(vips REQUIRED IMPORTED_TARGET "vips-cpp")
-
-# Remove libc++ from pkg-config, because it conflicts with multithreaded
-# libc++ from emscripten
-# \/ UNCOMMENT TO FIX \/
-# get_target_property(vips_libraries PkgConfig::vips INTERFACE_LINK_LIBRARIES)
-# list(FILTER vips_libraries EXCLUDE REGEX [[libc\+\+]])
-# set_target_properties(PkgConfig::vips PROPERTIES
-#     INTERFACE_LINK_LIBRARIES "${vips_libraries}")
-# /\ UNCOMMENT TO FIX /\
+pkg_check_modules(VIPS vips-cpp REQUIRED)
 
 # Add missing library directory
-target_link_directories(PkgConfig::vips INTERFACE ${vips_LIBRARY_DIRS})
-target_link_libraries(encre PRIVATE PkgConfig::vips)
+target_include_directories(encre PRIVATE ${VIPS_INCLUDE_DIRS})
+target_link_libraries(encre PRIVATE ${VIPS_STATIC_LDFLAGS})

(the change in CMakePresets.json isn't actually necessary, it became redundant with the use of VIPS_STATIC_LDFLAGS)

@DDoS
Copy link
Author

DDoS commented Oct 19, 2024

Thanks, I'll give it a try later this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants