Skip to content

Use a Shared build for cppinterop while building for wasm #375

New issue

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

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

Already on GitHub? # to your account

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Dec 5, 2024

Git diff can sometimes look to be pretty challenging to understand.

But what has been done is simply

if(EMSCRIPTEN)
  // new logic for shared build
 else()
   // all logic on master moved here
endif()

@anutosh491
Copy link
Collaborator Author

The logic for the shared build is simply this

  set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
  set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS "-s SIDE_MODULE=1")
  set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS "-s SIDE_MODULE=1")
  set(CMAKE_STRIP FALSE)

  add_llvm_library(clangCppInterOp
    SHARED

    CppInterOp.cpp
    DynamicLibraryManager.cpp
    DynamicLibraryManagerSymbol.cpp
    Paths.cpp

    # Additional libraries from Clang and LLD
    LINK_LIBS
    clangInterpreter
  )
  1. The first few lines are essential configurations required for a shared build as noted here (SHARED / dynamic library with CMake emscripten-core/emscripten#15276 (comment))

This way of configuring has been used in projects like pyodide, emscripten-forge where building a shared library is essential.

  1. add_llvm_library itself provides the shared option promoting a shared build. This should give us a libclangCppInterOp.so.19.1 and libclangCppInterOp.so

@anutosh491 anutosh491 marked this pull request as draft December 5, 2024 10:49
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.62%. Comparing base (7038fe7) to head (1092ad0).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #375   +/-   ##
=======================================
  Coverage   70.62%   70.62%           
=======================================
  Files           9        9           
  Lines        3500     3500           
=======================================
  Hits         2472     2472           
  Misses       1028     1028           

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Dec 5, 2024

Another major reason as to why I am not in favour of using different configs while building LLVM (clang, lld) for webassembly.

Focus on the error here

wasm-ld: error: unknown file type: /home/runner/work/CppInterOp/CppInterOp/llvm-project/build/lib/libclangInterpreter.a(DeviceOffload.cpp.o)
em++: error: '/home/runner/work/CppInterOp/CppInterOp/emsdk/upstream/bin/wasm-ld @/tmp/emscripten_y2obwybd.rsp.utf-8' failed (returned 1)
make[2]: *** [lib/Interpreter/CMakeFiles/clangCppInterOp.dir/build.make:245: lib/libclangCppInterOp.so.19.1] Error 1
make[1]: *** [CMakeFiles/Makefile2:351: lib/Interpreter/CMakeFiles/clangCppInterOp.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

What is happening is the default archiver being used is llvm-ar (which shouldn't be the case while building for webassembly using emscripten)

  1. As you can see, we have these warning here
em++: warning: /Users/runner/work/CppInterOp/CppInterOp/llvm-project/build/lib/libclangInterpreter.a: archive is missing an index; Use emar when creating libraries to ensure an index is created [-Wemcc]
em++: warning: /Users/runner/work/CppInterOp/CppInterOp/llvm-project/build/lib/libclangInterpreter.a: adding index [-Wemcc]

  1. Now if you don't build using emar but using llvm-ar, the compiled objects are not what we want.
// so if we extract object files first
> emar x libclangInterpreter.a 

// then check file type
// the file type is wrong.
// expected is WebAssembly object file
> file DeviceOffload.cpp.o
DeviceOffload.cpp.o: Mach-O 64-bit object arm64
  1. So basically while building llvm we need to supply the correct toolchain

i) Like what I did locally

-DCMAKE_TOOLCHAIN_FILE=/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-build/lib/python3.13/site-
packages/emsdk/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake

ii) What emscripten-forge does (https://github.com/emscripten-forge/recipes/actions/runs/12139306429/job/33846640952#step:9:216)

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Dec 5, 2024

Xeus-cpp's CI works perfectly now

  1. I made the exact same changes (added as a patch) on cppinterop's recipe on emscripten-forge to basically attempt a shared build (Attempt to use a shared build for CppInterOp emscripten-forge/recipes#1454)
  2. Then used it in xeus-cpp's CI and as expected the undefined symbols occur no more (Remove flag for ignoring undefined symbols for a wasm build xeus-cpp#190)

As I said above we should stick to 1 llvm build config here.

  1. I would do it exactly as we did it for xeus-cpp.
  2. We need something that sticks to emscripten standards (using emcmake, emmake and the emscripten toolchain) which is the whole point behind hosting stuff on emscripten-forge.
  3. I would just remove zlib and add llvm 19.1.5 here (https://github.com/compiler-research/CppInterOp/blob/main/environment-wasm.yml) and use it for the wasm build for cppinterop if given a choice.

@@ -130,6 +155,45 @@ if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
message(STATUS "Found supported version: LLVM ${LLVM_PACKAGE_VERSION}")
message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")

## Find supported LLD only while building for webassembly against emscripten
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only find lld when building against emscripten. This comment just conveys the same.

@mcbarton mcbarton marked this pull request as ready for review December 12, 2024 19:53
@mcbarton mcbarton self-requested a review December 12, 2024 19:53
@mcbarton mcbarton merged commit 4959cce into compiler-research:main Dec 12, 2024
45 checks passed
@anutosh491 anutosh491 deleted the shared_cppinterop branch December 13, 2024 04:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regarding lot of undefined symbols while building xeus-cpp against emscripten
2 participants