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

Add CI for WASI threads #1819

Merged

Conversation

eloparco
Copy link
Contributor

No description provided.

@eloparco
Copy link
Contributor Author

Not sure why the compilation on macos-latest job is failing, it works properly when I try it directly from my forked repo https://github.com/eloparco/wasm-micro-runtime/actions/runs/3713650979/jobs/6296822730.

@@ -210,7 +210,7 @@ jobs:
#$AOT_BUILD_OPTIONS,
]
os: [macos-latest]
wasi_sdk_release: ["https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-12/wasi-sdk-12.0-macos.tar.gz"]
wasi_sdk_release: ["https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-17/wasi-sdk-17.0-macos.tar.gz"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder if we want to continue building for 12 to ensure backward compatibility? Or do we want to drop 12 and ask users to migrate to >= 17 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried wasi-sdk version 12 and wasi-libc branch wasi-sdk-17 here but the build failed.
It was not immediate to find a compatible combination of wasi-sdk and wasi-libc for the different targets (on ubuntu I'm using wasi-sdk version 16 in this PR since version 17 was not working).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a little confused here. It seems the CI depends on the combination of wasi-sdk-X + a specific release of wasi-libc now. Is there a plan that a future wasi-sdk release include everything and make the local combination not necessary?

@eloparco eloparco force-pushed the eloparco/wasi-threads-ci branch 2 times, most recently from 2d39fc4 to abe0053 Compare December 19, 2022 14:24
@eloparco
Copy link
Contributor Author

It looks like all jobs are succeeding now, using wasi-sdk version 16 and wasi-libc with branch wasi-sdk-17 on all targets.

@eloparco
Copy link
Contributor Author

eloparco commented Dec 19, 2022

Actually, looking at the output (and at some tests I was doing locally) I see a regression:

New thread ID: 1, starting parameter: 52
Updated value: 8
Thread completed, new value: 0, thread id: 0

The new value and thread id are wrong.

I'm adding

LINKER:--export=malloc
LINKER:--export=free

in samples/wasi-threads/wasm-apps/CMakeLists.txt, not sure if I need to change anything else.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@eloparco Shall I merge the PR?

@@ -29,6 +29,8 @@ function (compile_sample SOURCE_FILE)
LINKER:--export=__data_end
LINKER:--shared-memory,--max-memory=1966080
LINKER:--export=wasi_thread_start
LINKER:--export=malloc
LINKER:--export=free
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I was getting this error without the additional exports:

[09:52:39:510 - 101150580]: Memory instantiate success.
[09:52:39:614 - 101150580]: Error: app heap is corrupted, if the wasm file is compiled by wasi-sdk-12.0 or higher version, please add -Wl,--export=malloc -Wl,--export=free to export malloc and free functions. If it is compiled by asc, please add --exportRuntime to export the runtime helpers.
[09:52:39:618 - 101150580]: thread manager error: failed to allocate aux stack space for new thread
[09:52:39:620 - 101150580]: Failed to spawn a new thread
[09:52:39:623 - 101150580]: /Users/eloparco/dev/forks/wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_fast.c, line 3959, meet an exception Exception: app heap corrupted
Exception: app heap corrupted

Copy link
Collaborator

Choose a reason for hiding this comment

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

the aux stack allocation should be disabled for wasi-threads because it's libc's responsibility to allocate stack for new threads.

no_pthread.c seems broken. its wasi_thread_start should either update the stack pointer global or avoid using C stack.

Copy link
Contributor Author

@eloparco eloparco Dec 20, 2022

Choose a reason for hiding this comment

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

the aux stack allocation should be disabled for wasi-threads because it's libc's responsibility to allocate stack for new threads.

https://github.com/bytecodealliance/wasm-micro-runtime/blob/dev/wasi_threads/core/config.h#L172

no_pthread.c seems broken. its wasi_thread_start should either update the stack pointer global or avoid using C stack.

How do I use the app heap? if I don't export the malloc and free functions I get Error: app heap is corrupted

Copy link
Contributor

Choose a reason for hiding this comment

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

According to this,

app heap and libc heap: the heap to allocate memory for wasm app, note that app heap is created only when the malloc/free functions (or __new/__release functions for AssemblyScript) are not exported and runtime can not detect the libc heap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've read that one already, but if I don't export malloc and free I'm getting the corruption error

Copy link
Collaborator

Choose a reason for hiding this comment

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

the aux stack allocation should be disabled for wasi-threads because it's libc's responsibility to allocate stack for new threads.

https://github.com/bytecodealliance/wasm-micro-runtime/blob/dev/wasi_threads/core/config.h#L172

#1799 (comment)

no_pthread.c seems broken. its wasi_thread_start should either update the stack pointer global or avoid using C stack.

How do I use the app heap? if I don't export the malloc and free functions I get Error: app heap is corrupted

wamr doesn't need to (and shouldn't) use the heap (app heap or libc heap) at least for this purpose (stack allocation for wasi-threads) because the stack is managed inside the module itself. (usually by libc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Threads implementation in WAMR is not fully completed yet. One of the things is to always (regardless of compilation flags) use on-heap aux stack allocation. I have change for that already but didn't test it yet; I'll make the change public beginning of next month when I'm back from holidays, until then I'd suggest waiting with merging this PR.

@eloparco
Copy link
Contributor Author

@wenyongh let's wait for @loganek's feedback before merging

@lum1n0us
Copy link
Collaborator

BTW, is there an issue to track the compilation error in https://github.com/eloparco/wasm-micro-runtime/actions/runs/3730554354/jobs/6327794669?

@wenyongh
Copy link
Contributor

@eloparco Wondering why you use wasi-sdk-16.0 for the CI and use wasi-sdk-17.0 for wasi-thread sample? Why not use wasi-sdk-17.0 for all?
We are upgrading the toolkits (llvm, wasi-sdk, emsdk, binaryen and so on), and are considering upgrade wasi-sdk to 17.0 directly.

@eloparco
Copy link
Contributor Author

@eloparco Wondering why you use wasi-sdk-16.0 for the CI and use wasi-sdk-17.0 for wasi-thread sample? Why not use wasi-sdk-17.0 for all?

@wenyongh That's what I tried first, but it was failing on ubuntu/android: https://github.com/eloparco/wasm-micro-runtime/actions/runs/3703249930/jobs/6274621343

BTW, is there an issue to track the compilation error in https://github.com/eloparco/wasm-micro-runtime/actions/runs/3730554354/jobs/6327794669?

@lum1n0us Not yet, should I create one in this repo?

@loganek
Copy link
Collaborator

loganek commented Dec 21, 2022

@eloparco Wondering why you use wasi-sdk-16.0 for the CI and use wasi-sdk-17.0 for wasi-thread sample? Why not use wasi-sdk-17.0 for all?

@wenyongh That's what I tried first, but it was failing on ubuntu/android: https://github.com/eloparco/wasm-micro-runtime/actions/runs/3703249930/jobs/6274621343

@eloparco are you going to fix the issue? Or should we at least have an issue to upgrade to v17?

@eloparco
Copy link
Contributor Author

@loganek @lum1n0us I'll create an issue to track the upgrade to v17, putting links to the failing jobs

@wenyongh
Copy link
Contributor

@eloparco Wondering why you use wasi-sdk-16.0 for the CI and use wasi-sdk-17.0 for wasi-thread sample? Why not use wasi-sdk-17.0 for all?

@wenyongh That's what I tried first, but it was failing on ubuntu/android: https://github.com/eloparco/wasm-micro-runtime/actions/runs/3703249930/jobs/6274621343

Got it, I tested building wasi-libc and other wasm app with wasi-sdk-17 on Ubuntu-20.04, and got the same errors:

/opt/wasi-sdk/bin/clang: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by /opt/wasi-sdk/bin/clang)
/opt/wasi-sdk/bin/clang: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by /opt/wasi-sdk/bin/clang)
/opt/wasi-sdk/bin/clang: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /opt/wasi-sdk/bin/clang)

It seems that wasi-sdk-17 requires that libc-2.32 or newer version is installed, while by default in Ubuntu-20.04 libc-2.31 is installed. It may work in Ubuntu-22.04. But it seems that we had better just upgrade scripts to support wasi-sdk-16 now.

@@ -323,6 +323,17 @@ jobs:
sudo tar -xzf wabt-1.0.24-*.tar.gz
sudo mv wabt-1.0.24 wabt

- name: build wasi-libc (needed for wasi-threads)
run: |
git clone --branch wasi-sdk-17 https://github.com/WebAssembly/wasi-libc
Copy link
Contributor

Choose a reason for hiding this comment

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

Branch wasi-sdk-17 isn't found in the wasi-libc repo, seems that main branch is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a tag https://github.com/WebAssembly/wasi-libc/tree/wasi-sdk-17.

I'd prefer to pin the version/tag rather than using main. Unless we want to use the current latest commit from main and we checkout that specific one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember that the combination wasi-sdk-16 + main from wasi-libc was failing on Ubuntu.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. So shall we merge the PR now? @loganek, are there any comments from you? Thanks.

Copy link
Contributor Author

@eloparco eloparco Dec 22, 2022

Choose a reason for hiding this comment

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

I think so, to recap:

  • we're using wasi-sdk-16 because wasi-sdk-17 would require a newer version of libc wrt the one on Ubuntu-20.04 used in the pipeline (the alternative would be to update the Ubuntu image)
  • --export=malloc and --export=free are used here otherwise the pipeline would fail when executing the wasi-threads samples (the alternative would be to comment the assert in samples/wasi-threads/wasm-apps/no_pthread.c); the behavior will be fixed in a new PR

@@ -266,6 +266,17 @@ jobs:
sudo tar -xzf wabt-1.0.24-*.tar.gz
sudo mv wabt-1.0.24 wabt

- name: build wasi-libc (needed for wasi-threads)
run: |
git clone --branch wasi-sdk-17 https://github.com/WebAssembly/wasi-libc
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -268,6 +268,17 @@ jobs:
sudo tar -xzf wabt-1.0.24-*.tar.gz
sudo mv wabt-1.0.24 wabt

- name: build wasi-libc (needed for wasi-threads)
run: |
git clone --branch wasi-sdk-17 https://github.com/WebAssembly/wasi-libc
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@wenyongh wenyongh merged commit 7d19b22 into bytecodealliance:dev/wasi_threads Dec 22, 2022
@loganek loganek mentioned this pull request Jan 6, 2023
19 tasks
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Update wasi-sdk from 12.0 to 16.0 in CI
Build wasi-libc and build wai-threads sample in CI
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.

6 participants