Skip to content

DO NOT MERGE: WebAssembly standalone support#103

Open
jerbob92 wants to merge 51 commits into
bblanchon:masterfrom
jerbob92:wasm-standalone
Open

DO NOT MERGE: WebAssembly standalone support#103
jerbob92 wants to merge 51 commits into
bblanchon:masterfrom
jerbob92:wasm-standalone

Conversation

@jerbob92

Copy link
Copy Markdown
Contributor

@bblanchon I promised to come back to you when I finished my WebAssembly implementation, so here it is! It's not ready to merge this in, since it will break support for the web build, but it is possible to use this as a base to create 2 WASM builds: web and standalone.

This contains the following changes:

  • A patch for PDFium to add support for the callbacks, this was necessary because the runtime that I use (Wazero) doesn't support adding functions to the function table dynamically, so it basically adds these function pointers during compile time and that calls back into the host function of the runtime. This does mean that you can't just start loading the WebAssembly file on any runtime, since you do have to implement these host methods, but the same could be said for WASI and the host calls that the web builds add. We might want to add some documentation on these methods though?
  • A patch for Emscripten to implement the WASI methods in the standalone build to make file operations work
  • An update to the latest Emscripten (you probably want to include this for the web build too)
  • A switch between debug/non-debug builds that also includes optimizations (you might want to partially include this for the web build too)
  • A change in the build step that also includes other methods that should be exposed (you might want to partially include this for the web build too)

The thing that I'm not really happy with is how the Emscripten patch is applied, but I couldn't get it to work in another way, since emsdk/upstream/emscripten is not a git repo.

@bblanchon

Copy link
Copy Markdown
Owner

Hi @jerbob92,

Thank you very much for sharing this!

I didn't expect that we'd have to patch emscriptem and add a new features PDF.
I think this is out of the scope of this repository: the goal here is to provide a pre-built PDFium, not extend PDFium.
Do you think these changes could be integrated upstream?

I'll try to cherry-pick the changes you recommend right away.
I originally downgraded emscriptem in f09df4a to work around the error "Uncaught RuntimeError: unreachable"; hopefully, this bug is gone.

Best regards,
Benoit

@jerbob92

jerbob92 commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

I think Emscripten is not going to accept the patch in this state because it only implements what PDFium uses, nothing else.

And regarding extending PDFium: I don't think there is a way to work around this, to use function pointers in WebAssembly, the pointer needs to exist during compile time or you need to add the pointer to a method during runtime, which many runtimes don't support, that's why I had to implement various callbacks in the PDFium patch and then implement the callback logic on the runtime side. So I wouldn't really see it as extending PDFium, this is just a way to make all functionality work on in WebAssembly.

I originally downgraded emscriptem in f09df4a to work around the error "Uncaught RuntimeError: unreachable"; hopefully, this bug is gone.

I think this was related to the memory allocator that has since been disabled.

@jerbob92

Copy link
Copy Markdown
Contributor Author

@bblanchon small warning, free and malloc don't seem to export by default anymore in the latest Emscripten, I had to add them to EXPORTED_FUNCTIONS myself. Just in case you want to upgrade Emscripten in the future. The build works fine but people using the Webassembly build will need access to free and malloc, although I'm not sure if that's also the case for JS users.

Comment thread steps/04-install-extras.sh Outdated
cd emsdk
./emsdk install ${EMSDK_VERSION:-latest}
./emsdk activate ${EMSDK_VERSION:-latest}
./emsdk install ${EMSDK_VERSION:-4.0.11}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have changed this back to a static version, to make sure that emscripten.patch applies.

Comment thread steps/05-configure.sh
echo "pdf_is_standalone = true"
echo "pdf_use_partition_alloc = false"
echo "target_cpu = \"$TARGET_CPU\""
if [ "$TARGET_CPU" == "wasm-standalone" ]; then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This felt like the best solution to me right now to prevent a lot of patches, let me know what you think.

@jerbob92

Copy link
Copy Markdown
Contributor Author

@bblanchon I have updated the MR to make it possible to have this next to the normal wasm build, this introduces the CPU target wasm-standalone, I'm not sure if it's useful for anyone else, but it would be nice to have this inside the main repo.

if [ "$CPU" == "wasm-standalone" ]; then
pushd "$PWD/upstream/emscripten"
patch -p1 --forward < "$PATCHES/wasm/emscripten.patch" || true
rm -Rf cache

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is to make sure emscripten recreates "prebuilt" libraries.

@bblanchon

Copy link
Copy Markdown
Owner

Hi @jerbob92,

I'm sorry for not getting back to you sooner.
I only found the time to take a quick glance at this PR, and what struck me immediately is the huge patch of emscriptem.
I don't think it's the role of the present project to maintain such a big change in emscripten; I know I would be unable to maintain the code.
Can't this be moved upstream?

Best regards,
Benoit

@jerbob92

Copy link
Copy Markdown
Contributor Author

Thanks for checking @bblanchon! The emscripten patch is actually from emscripten-core/emscripten#18285
It's not merged in yet, but work is being done to merge this upstream in smaller parts. It doesn't cost me a lot of time to maintain, I mostly just merge in the latest tag into my implement-more-syscalls-for-standalone branch and that's it.

@bblanchon

Copy link
Copy Markdown
Owner

I'd be more comfortable if we could wait for this PR to be merged.
Do you think it has a chance of being merged anytime soon?

@jerbob92

Copy link
Copy Markdown
Contributor Author

I'd be more comfortable if we could wait for this PR to be merged. Do you think it has a chance of being merged anytime soon?

It's been moving forward really slowly since 2022, so probably not soon.

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.

3 participants