Frame tests for loading shared objects for the emscripten build #548

anutosh491 · 2025-04-02T11:17:45Z

Description

Now that llvm/llvm-project#133037 has been merged, we should be able to test out loading shared libraries for the emscripten build.

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

I have read the contribution guide recently

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

mcbarton · 2025-04-02T11:37:37Z

@anutosh491 Just a couple of questions. Once this PR is ready will we be able to run the DynamicLibraryManagerTest Sanity test? Currently its skipped.

Does this PR have any relation to this PR? #308

anutosh491 · 2025-04-02T11:44:01Z

Explaining the use-case/roadmap here

Symengine a Computer Algebra System with 1.2k stars wants us to host a link in their Readme so that users there can try it as a REPL in the browser. Check this

Steps to achieve this

Add a milestone label for [clang-repl] Implement LoadDynamicLibrary for clang-repl wasm use cases llvm/llvm-project#133037 . I have already cherry picked the relevant commit. @vgvassilev should be able to help us out here.
Move to llvm 20 (Upgrade CppInterOp to support llvm 20 #491)
Take this PR in after any review involved (and probably make a release whenever we are happy)
SymEngine used #pragma cling load back in the days to integrate with xeus-cling
https://github.com/symengine/symengine/blob/master/symengine/symengine_config_cling.h.in#L5C1-L5C20
We can add a similar file having something like

#include "clang/Interpreter/CppInterOp.h"
static bool _symengine_loaded = []() {
    Cpp::LoadLibrary(@SYMENGINE_CPPINTEROP_LIBRARY_PATH@);
    return true;
}();

Included when we build using these definitions #if defined(__CLANG_REPL__) && defined(__EMSCRIPTEN__)

Was able to get this working locally !!!

This involves only having symengine in our wasm environment and no explicit linking/loading from our side. Any symengine header included would also load libsymengine.so .

So to provide the link to symengine, we would just ask the maintainers to fork the template library (https://github.com/jupyterlite/xeus-lite-demo) under their symengine org (and probably ask them to name it Symengine/xeus-cpp-symengine-demo or whatever) . Here in the template they would just need to update the environment and add symengine to get the above working.

anutosh491 · 2025-04-02T11:44:27Z

Once this PR is ready will we be able to run the DynamicLibraryManagerTest Sanity test?

Absolutely. This PR fixes that.

EDIT: Actually to be fair not the Sanity test. The sanity test is not catered to the emscripten build.But we shall be able to add a custom test suitable for the emscripten build just like what I added (look at the test I added in the pr)

The sanity test has

std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr);
  llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
  Cpp::AddSearchPath(Dir.str().c_str());

Things like llvm::sys etc would not work for emscripten. Also there is no real concept of search paths in wasm ( we know where the preload the shared lib in the MEMFS so we don't have to search for it )

Does this PR have any relation to this PR? #308

The library autoloader PR is not focusing on loading dynamic libs while building against emscripten (or basically for our wasm use case) as per my knowledge. So the idea might have some relation but the use case is different.

codecov · 2025-04-02T12:23:10Z

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 76.36%. Comparing base (03135b6) to head (c2dbd47).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/Interpreter/CppInterOpInterpreter.h	28.57%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #548      +/-   ##
==========================================
- Coverage   76.45%   76.36%   -0.10%     
==========================================
  Files           9        9              
  Lines        3648     3655       +7     
==========================================
+ Hits         2789     2791       +2     
- Misses        859      864       +5

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOpInterpreter.h	`78.64% <28.57%> (-1.90%)`	⬇️

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOpInterpreter.h	`78.64% <28.57%> (-1.90%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

anutosh491 · 2025-05-14T12:53:49Z

Just in case, anyone would like to try this through lite.

make a custom_module.cpp

#include <iostream>

extern "C" {
    void my_custom_function() {
        std::cout << "Hello from my_custom_function!" << std::endl;
    }
}

compile it as a Side Module

emcc custom_module.cpp -s SIDE_MODULE=1 -O3 -o custom_module.so

Load it through Jupyterlite UI at runtime (through the upload button at the left... shown by an arrow)
Once you have it there, try using it

lib/Interpreter/CppInterOpInterpreter.h

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOpInterpreter.h

unittests/CppInterOp/CMakeLists.txt

mcbarton · 2025-05-14T17:58:34Z

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 76.28%. Comparing base (28ba16e) to head (54e0e14).

Files with missing lines Patch % Lines
lib/Interpreter/CppInterOpInterpreter.h 28.57% 5 Missing ⚠️
Additional details and impacted files
🚀 New features to boost your workflow:

The tests don't cover all the lines of the patch. Please fix this.

anutosh491 · 2025-05-14T18:04:11Z

The tests don't cover all the lines of the patch. Please fix this.

Weird

The support for loading shared libraries in llvm was added in llvm 20
So if you see the test caters only to

TEST(DynamicLibraryManagerTest, BasicSymbolLookup) {
#ifndef EMSCRIPTEN
  GTEST_SKIP() << "This test is only intended for Emscripten builds.";
#else
  #if CLANG_VERSION_MAJOR < 20
    GTEST_SKIP() << "Support for loading shared libraries was added in LLVM 20.";
  #endif
#endif

So the added test on any xx-xx-clang-rep-20-emscripten would only pass if it goes through the mechanism added.

I don't know how the above codecov report works but the test added is literally to address this :)

EDIT : Hmmm, I see the codecov/patch && codecov/project fail even before the emscripten test is run ( the emscripten builds were going on and these jobs already failed)

EDIT 2 : This should confirm the those patches are tested http://github.com/compiler-research/CppInterOp/actions/runs/15027746990/job/42232690523?pr=548#step:9:708

mcbarton · 2025-05-14T18:17:59Z

The tests don't cover all the lines of the patch. Please fix this.

Weird

The support for loading shared libraries in llvm was added in llvm 20

So if you see the test caters only to
TEST(DynamicLibraryManagerTest, BasicSymbolLookup) {
#ifndef EMSCRIPTEN
  GTEST_SKIP() << "This test is only intended for Emscripten builds.";
#else
  #if CLANG_VERSION_MAJOR < 20
    GTEST_SKIP() << "Support for loading shared libraries was added in LLVM 20.";
  #endif
#endif
So the added test on any xx-xx-clang-rep-20-emscripten would only pass if it goes through the mechanism added.

I don't know how the above codecov report works but the test added is literally to address this :)

Just had a look and this seems to be a result of us moving away from ifdef __EMSCRIPTEN__ . Given the codecov is run on a native platform it won't ever be able test triple.isWasm() == true . @vgvassilev can decide what to do with that information is his review

We probably need some way to have code coverage number for Emscripten if possible, but that is another issue to this PR entirely.

anutosh491 · 2025-05-14T18:22:37Z

it won't ever be able test triple.isWasm() == true

Arghh, I thought using target opts here is a more organic solution but if we can't then probably we can't :\
(should codecov be respected in every case ? if yes, can we somehow skip it for patches we know shouldn't be covered)

anutosh491 · 2025-05-21T06:51:02Z

Hey @vgvassilev

The other day I addressed your reviews here but we see this after I got rid of the hardcoding.

#548 (comment)

To which I make this comment #548 (comment) saying what you suggested seems the organic way to get this but it might not be covered through codecov.

Let us know what you think !

vgvassilev

LGTM!

vgvassilev · 2025-05-21T06:54:29Z

The tests don't cover all the lines of the patch. Please fix this.

Weird

The support for loading shared libraries in llvm was added in llvm 20

So if you see the test caters only to
TEST(DynamicLibraryManagerTest, BasicSymbolLookup) {
#ifndef EMSCRIPTEN
  GTEST_SKIP() << "This test is only intended for Emscripten builds.";
#else
  #if CLANG_VERSION_MAJOR < 20
    GTEST_SKIP() << "Support for loading shared libraries was added in LLVM 20.";
  #endif
#endif
So the added test on any xx-xx-clang-rep-20-emscripten would only pass if it goes through the mechanism added.

I don't know how the above codecov report works but the test added is literally to address this :)
Just had a look and this seems to be a result of us moving away from ifdef __EMSCRIPTEN__ . Given the codecov is run on a native platform it won't ever be able test triple.isWasm() == true . @vgvassilev can decide what to do with that information is his review

We probably need some way to have code coverage number for Emscripten if possible, but that is another issue to this PR entirely.

We should move away from that hardcoding but in a separate PR that covers the tests.

github-actions bot reviewed Apr 2, 2025

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Outdated Show resolved Hide resolved

mcbarton force-pushed the shared_libs branch from 9b633fd to 6b666b2 Compare April 2, 2025 11:33

anutosh491 force-pushed the shared_libs branch from 6b666b2 to 040dd06 Compare May 12, 2025 02:10

github-actions bot reviewed May 12, 2025

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Show resolved Hide resolved

anutosh491 force-pushed the shared_libs branch from e056599 to b116512 Compare May 12, 2025 02:18

github-actions bot reviewed May 12, 2025

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Show resolved Hide resolved

anutosh491 force-pushed the shared_libs branch from b116512 to 192962a Compare May 12, 2025 03:07

anutosh491 marked this pull request as ready for review May 12, 2025 03:51

anutosh491 force-pushed the shared_libs branch from 82b42e9 to bd82e79 Compare May 12, 2025 04:57

github-actions bot reviewed May 12, 2025

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Show resolved Hide resolved

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Show resolved Hide resolved

anutosh491 force-pushed the shared_libs branch from 9acdcba to 72dc3f9 Compare May 12, 2025 05:12

anutosh491 force-pushed the shared_libs branch from 72dc3f9 to 7ec7c90 Compare May 14, 2025 12:56

vgvassilev reviewed May 14, 2025

View reviewed changes

lib/Interpreter/CppInterOpInterpreter.h Outdated Show resolved Hide resolved

lib/Interpreter/CppInterOpInterpreter.h Outdated Show resolved Hide resolved

github-actions bot reviewed May 14, 2025

View reviewed changes

lib/Interpreter/CppInterOpInterpreter.h Show resolved Hide resolved

anutosh491 commented May 14, 2025

View reviewed changes

unittests/CppInterOp/CMakeLists.txt Show resolved Hide resolved

anutosh491 force-pushed the shared_libs branch from 1ef2499 to afaaee1 Compare May 14, 2025 17:59

anutosh491 added 2 commits May 21, 2025 12:21

Frame tests for loading shared objects and fetching symbol address

3735c88

use TargetOpts

c2dbd47

anutosh491 force-pushed the shared_libs branch from afaaee1 to c2dbd47 Compare May 21, 2025 06:51

vgvassilev approved these changes May 21, 2025

View reviewed changes

anutosh491 merged commit 6951212 into compiler-research:main May 21, 2025
72 of 74 checks passed

anutosh491 deleted the shared_libs branch May 21, 2025 07:55

Frame tests for loading shared objects for the emscripten build #548

Frame tests for loading shared objects for the emscripten build #548

Conversation

anutosh491 commented Apr 2, 2025

Description

Type of change

Testing

Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcbarton commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented Apr 2, 2025

Uh oh!

anutosh491 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anutosh491 commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcbarton commented May 14, 2025

Codecov Report

Uh oh!

anutosh491 commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcbarton commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented May 21, 2025

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

mcbarton commented Apr 2, 2025 •

edited

Loading

anutosh491 commented Apr 2, 2025 •

edited

Loading

codecov bot commented Apr 2, 2025 •

edited

Loading

anutosh491 commented May 14, 2025 •

edited

Loading

mcbarton commented May 14, 2025 •

edited

Loading

anutosh491 commented May 14, 2025 •

edited

Loading