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

Support build target wasm32-unknown-unknown in clang when ENABLE_THREADS=OFF #57

Closed

Conversation

ChanTsune
Copy link
Contributor

@ChanTsune ChanTsune commented Jul 27, 2023

Thanks for the review #56 yesterday!

I tried another approach, could you please review this one?

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build was run locally and without warnings or errors
  • All previous and new tests pass

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming, typo fix)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

$ mkdir build && cd build
$ cmake .. -DENABLE_THREADS=OFF
$ make liblzma
[  0%] Building C object CMakeFiles/liblzma.dir/src/common/tuklib_physmem.c.obj                                       
[  1%] Building C object CMakeFiles/liblzma.dir/src/liblzma/check/check.c.obj                                         
In file included from /xz/src/liblzma/check/check.c:13:                                                               
In file included from /xz/src/liblzma/check/check.h:16:                                                               
In file included from /xz/src/liblzma/common/common.h:17:
In file included from /xz/src/common/mythread.h:84:
/wasi-sysroot/include/signal.h:2:2: error: "wasm lacks signal support; to enable minimal signal emulation, compile with -D_WASI_EMULATED_SIGNAL and link with -lwasi-emulated-signal"
#error "wasm lacks signal support; to enable minimal signal emulation, \
 ^
In file included from /xz/src/liblzma/check/check.c:13:
In file included from /xz/src/liblzma/check/check.h:16:
In file included from /xz/src/liblzma/common/common.h:17:
/xz/src/common/mythread.h:87:33: error: unknown type name 'sigset_t'
mythread_sigmask(int how, const sigset_t *restrict set,
                                ^
/xz/src/common/mythread.h:88:3: error: unknown type name 'sigset_t'
                sigset_t *restrict oset)
                ^
/xz/src/common/mythread.h:90:12: error: call to undeclared function 'sigprocmask'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        int ret = sigprocmask(how, set, oset);
                  ^
4 errors generated.
make[3]: *** [CMakeFiles/liblzma.dir/build.make:90: CMakeFiles/liblzma.dir/src/liblzma/check/check.c.obj] Error 1
make[2]: *** [CMakeFiles/Makefile2:139: CMakeFiles/liblzma.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:146: CMakeFiles/liblzma.dir/rule] Error 2
make: *** [Makefile:179: liblzma] Error 2

Related Issue URL:
#56

What is the new behavior?

  • Changed to exclude signal functions not supported by WebAssembly, using the predefined __wasm__ macro when the build target is set to wasm32 with clang.
    This change allows liblzma to be built with the platform-independent wasm32-uknown-unknown target.
    I believe this exclusion will work in the same way as the build when targeting Windows, so it will minimize unexpected changes.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Build tested on docker

Dockerfile

FROM ghcr.io/webassembly/wasi-sdk:latest

RUN apt update && apt install -y git

RUN git clone https://github.com/ChanTsune/xz.git

RUN mkdir -p ./xz/build

RUN cd xz && git fetch && git switch feature/liblzma/wasm

WORKDIR /xz/build

RUN cmake .. -DENABLE_THREADS=OFF

RUN CFLAGS="-target wasm32-unknown-unknown" make liblzma
$ docker build -t xz .

If you need, you can see the predefined macros when targeting wasm32 in clang by following commands

$ clang -E -dM -target wasm32-unknown-unknown -x c /dev/null

or if you want to check with the latest wasi-sdk clang

$ docker image pull ghcr.io/webassembly/wasi-sdk
$ docker run --rm -i ghcr.io/webassembly/wasi-sdk clang-16 -E -dM -target wasm32-unknown-unknown -x c /dev/null

@JiaT75
Copy link
Contributor

JiaT75 commented Jul 27, 2023

Hi! Thanks again for the very detailed PR. I looked more into wasm signal support since at first I thought it was some sort of bug that the signal emulation did not define sigset_t or sigprocmask(). This seems intentional however so your PR is certainly needed for a successful port to web assembly.

What you have so far seems like it is enough for liblzma to build, but we also should support an xz port. This should be easy to add just by following the example of VMS in src/xz/signal.*. Since the only functions in xz that use mythread_sigmask() it should be enough to define signals_block() and signals_unblock() as no-ops in signal.h (and remove implementation from signal.c).

Let me know if you have questions or if something else is preventing us from building xz with wasi-sdk

@JiaT75
Copy link
Contributor

JiaT75 commented Jul 27, 2023

On second thought, will be more complicated that I initially thought since signals_init() needs to be disabled too. I will merge what you have once I make a fix for the xz side. Thanks for you contributions!

@ChanTsune
Copy link
Contributor Author

Thank you for your kind review!

I am honored to contribute to your project!

@Larhzu Larhzu closed this in 81db3b8 Aug 1, 2023
@JiaT75
Copy link
Contributor

JiaT75 commented Aug 1, 2023

@ChanTsune We would like to add you to our THANKS file in the repository but were not sure what your name was. If you are interested in being credited in the THANKS file please let us know the name you would like credited since we could not find it on your GitHub profile.

Thanks again for your contribution!

Larhzu pushed a commit that referenced this pull request Aug 1, 2023
signal.h in WASI SDK doesn't currently provide sigprocmask()
or sigset_t. liblzma doesn't need them so this change makes
liblzma and xzdec build against WASI SDK. xz doesn't build yet
and the tests don't either as tuktest needs setjmp() which
isn't (yet?) implemented in WASI SDK.

Closes: #57
See also: #56

(The original commit was edited a little by Lasse Collin.)
@ChanTsune
Copy link
Contributor Author

Thank you! I updated my profile name. I would appreciate it if you could add this name to the THANKS file!

@ChanTsune ChanTsune deleted the feature/liblzma/wasm branch September 25, 2023 04:12
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.

None yet

2 participants