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.

Sign up for GitHub

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

test: add tests for pthread API #369

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test: add tests for pthread API #369

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 50 additions & 6 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ test: run
# directory (keeping with the `wasi-libc` conventions).
OBJDIR ?= $(CURDIR)/build
DOWNDIR ?= $(CURDIR)/download
# Like the top-level `wasi-libc` Makefile, here we can decide whether to test
# with threads (`posix`) or without (`single`). IMPORTANT: the top-level include
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like yet another argument in favor of cmake

# directory must be built with the same value.
THREAD_MODEL ?= single

##### DOWNLOAD #################################################################

Expand Down Expand Up @@ -90,6 +94,14 @@ TESTS := \
$(LIBC_TEST)/src/functional/wcsstr.c \
$(LIBC_TEST)/src/functional/wcstol.c

ifeq ($(THREAD_MODEL), posix)
TESTS += \
$(LIBC_TEST)/src/functional/pthread_cond.c \
$(LIBC_TEST)/src/functional/pthread_tsd.c \
$(LIBC_TEST)/src/functional/tls_init.c \
$(LIBC_TEST)/src/functional/tls_local_exec.c
endif

# Part of the problem including more tests is that the `libc-test`
# infrastructure code is not all Wasm-compilable. As we include more tests
# above, this list will also likely need to grow.
Expand All @@ -116,23 +128,54 @@ ifeq ($(origin CC), default)
CC := clang
endif
LDFLAGS ?=
CFLAGS ?= --target=wasm32-wasi --sysroot=../sysroot
CFLAGS ?= --sysroot=../sysroot
# Always include the `libc-test` infrastructure headers.
CFLAGS += -I$(LIBC_TEST)/src/common

# Configure support for threads.
ifeq ($(THREAD_MODEL), single)
CFLAGS += --target=wasm32-wasi -mthread-model single
endif
ifeq ($(THREAD_MODEL), posix)
# Specify the tls-model until LLVM 15 is released (which should contain
# https://reviews.llvm.org/D130053).
CFLAGS += --target=wasm32-wasi-pthread -mthread-model posix -pthread -ftls-model=local-exec
# NOTE: `--export-memory` is invalid until https://reviews.llvm.org/D135898 is
Copy link
Member

Choose a reason for hiding this comment

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

The change related to being able to export or import the memory with a specific name --export-memory on its own (using the default name) I think has existed for a long time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was only able to see the --export-memory flag in a built-from-tree version of Clang. Should I be able to find it in some binary version of Clang?

Copy link
Member

Choose a reason for hiding this comment

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

Oh maybe you are right.. exporting of the memory was always just the default.

Remind me why you need to both import and export the memory? .. emscripten doesn't do that today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uh, well, not by choice:

  • we want to import the shared memory to avoid magic: the shared memory is created outside of the instance and passed in as an import
  • then, Wasmtime wants me to also export the memory because this is how the Wiggle integration figures out where buffers are at when a WASI function is called with a buffer as a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. that seems like unique concern of wasmtime and/or wiggle and perhaps we should at least make a note of that. Perhaps add --import-memory (which all runtimes will need) on the line above and then leave --export-memory on a line by itself with a comment? Perhaps something like "with the instnace-per-thread model we always need to import the memory, and under wasmtime we also need re-export this imported memory. The ability to both export and import a memory is only available in recent versions of wasm-ld".

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any need to make that assumption. no. If the runtime wants a static signal it could use the import of wasi_thread_create.

I'm not sure a runtime necessarily needs to know statically that a program is mulithreaded, though. A runtime could just implement wasi_thread_create and have it create a new thread without necessarily knowing up front couldn't it? This wouldn't be very useful without an imported shared memory though.... so we might want to specify that a program that calls wasi_thread_create needs to import at least one shared memory?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, "automatically create shared memory which satisfies imports" is a very wasi-threads specific behavior, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reading through this chain so far, I think @sbc100 has explained things well. We need to import a shared memory and in Wasmtime we will know to do so because of configuration that turns on Wasm threads and wasi-threads. My takeaway is that we could clarify the README a bit more in this regard but I do want to be a bit cautious about overspecifying to allow different runtimes to implement the configuration as they wish.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let me see how i can amend my implementation. after that experiment, maybe i will submit a README patch.

Copy link
Contributor

@yamt yamt Dec 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ok, let me see how i can amend my implementation. after that experiment, maybe i will submit a README patch.

done.

WebAssembly/wasi-threads#19 (comment)
yamt/toywasm@4d81846
WebAssembly/wasi-threads#20

# available in a released version of `wasm-ld`; this has not happened yet so
# a built-from-latest-source Clang is necessary.
LDFLAGS += -Wl,--import-memory,--export-memory,--max-memory=1048576
endif

# We want to be careful to rebuild the Wasm files if any important variables
# change. This block calculates a hash from key variables and writes a file
# (ENV_HASH = `build/env-<hash>`); any targets dependent on `ENV_HASH` will thus
# be rebuilt whenever `ENV_HASH` is modified, which is whenever `ENV` changes.
# The phony `env` target can be used to debug this.
ENV = "CC=$(CC);CFLAGS=$(CFLAGS);LDFLAGS=$(LDFLAGS);THREAD_MODEL=$(THREAD_MODEL)"
export ENV
ENV_HASH = $(OBJDIR)/env-$(shell echo $(ENV) | md5sum | cut -d ' ' -f 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

macOS doesn't have md5 command.

diff --git a/test/Makefile b/test/Makefile
index 7f99f62..d52e5c2 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -153,7 +153,7 @@ endif
 # The phony `env` target can be used to debug this.
 ENV = "CC=$(CC);CFLAGS=$(CFLAGS);LDFLAGS=$(LDFLAGS);THREAD_MODEL=$(THREAD_MODEL)"
 export ENV
-ENV_HASH = $(OBJDIR)/env-$(shell echo $(ENV) | md5sum | cut -d ' ' -f 1)
+ENV_HASH = $(OBJDIR)/env-$(shell echo $(ENV) | openssl md5 -r | cut -d ' ' -f 1 | cut -c 1-)
 $(ENV_HASH): | $(OBJDIRS)
        rm -f $(OBJDIR)/env-*
        echo $(ENV) > $@

$(ENV_HASH): | $(OBJDIRS)
rm -f $(OBJDIR)/env-*
echo $(ENV) > $@
env: $(ENV_HASH)
@echo ENV = "$$ENV"
@echo ENV_HASH = $(ENV_HASH)
cat $^
.PHONY: env

# Compile each selected test using Clang. Note that failures here are likely
# due to a missing `libclang_rt.builtins-wasm32.a` in the Clang lib directory.
# This location is system-dependent, but could be fixed by something like:
# $ sudo mkdir /usr/lib64/clang/14.0.5/lib/wasi
# $ sudo cp download/lib/wasi/libclang_rt.builtins-wasm32.a /usr/lib64/clang/14.0.5/lib/wasi/
build: download $(WASMS)
build: download $(WASMS) $(ENV_HASH)

$(WASMS): | $(OBJDIRS)
$(OBJDIR)/%.wasm: $(OBJDIR)/%.wasm.o $(INFRA_WASM_OBJS)
$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^
$(OBJDIR)/%.wasm: $(OBJDIR)/%.wasm.o $(INFRA_WASM_OBJS) $(ENV_HASH)
$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(filter-out $(ENV_HASH), $^)

$(WASM_OBJS): $(LIBC_TEST)/src/common/test.h | $(OBJDIRS)
$(OBJDIR)/%.wasm.o: $(LIBC_TEST)/src/%.c
$(OBJDIR)/%.wasm.o: $(LIBC_TEST)/src/%.c $(ENV_HASH)
$(CC) $(CFLAGS) -c -o $@ $<

$(OBJDIRS):
Expand All @@ -144,6 +187,7 @@ clean::
##### RUN ######################################################################

ENGINE ?= $(WASMTIME) run
ENGINE_FLAGS ?= --wasm-features threads --wasi-modules experimental-wasi-threads
ERRS:=$(WASMS:%.wasm=%.wasm.err)

# Use the provided Wasm engine to execute each test, emitting its output into
Expand All @@ -154,7 +198,7 @@ run: build $(ERRS)
$(ERRS): | $(OBJDIRS)

%.wasm.err: %.wasm
$(ENGINE) $< >$@
$(ENGINE) $(ENGINE_FLAGS) $<

clean::
rm -rf $(OBJDIR)/*/*.err
Expand Down