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

Consider including a pthread.h for all targets #501

Open
dicej opened this issue May 21, 2024 · 4 comments
Open

Consider including a pthread.h for all targets #501

dicej opened this issue May 21, 2024 · 4 comments

Comments

@dicej
Copy link
Contributor

dicej commented May 21, 2024

As part of the WASIp2 work, I changed the Makefile to produce a separate include directory for each target, only adding a pthread.h to the wasm32-wasi-threads one (and its wasm32-wasip1-threads alias). That was a change from the old behavior, where we had a single include directory for all targets, with the result that they all had pthread.h.

Lately, I've been helping with the WASI port of .NET, where pthread.h is used in the runtime library for all TARGET_UNIX platforms. That worked fine for older versions of wasi-sdk, since pthread.h was (accidentally?) available for the wasm32-wasi target, but doesn't work for wasi-sdk 22.

We can certainly change the #ifdef TARGET_UNIX conditionals that guard use of pthread.h to e.g. #if (defined TARGET_UNIX) && !(defined TARGET_WASI), but I'm wondering if it would be useful to include some form of pthread.h in the include directories for all targets in wasi-libc. This would make it easier for projects like .NET to upgrade the wasi-sdk version they're using, as well as make it easier to port other software that uses pthread.h but doesn't necessarily ever create more than one thread.

Thoughts?

@sunfishcode
Copy link
Member

I think it makes sense to include some form of pthread.h in the include directories for all targets. At one time I had the idea to exclude it so that applications wouldn't detect its presence and enable threads if they wouldn't actually be available, but I now think think that's less important than having pthread.h present to make it easier to port applications.

@sbc100
Copy link
Member

sbc100 commented May 21, 2024

Would we also then provide stub versions of the pthread APIs that fail at runtime (when not building with pthread support)?

This is what emscripten does BTW: https://github.com/emscripten-core/emscripten/blob/main/system/lib/pthread/library_pthread_stub.c

@dicej
Copy link
Contributor Author

dicej commented May 22, 2024

Would we also then provide stub versions of the pthread APIs that fail at runtime (when not building with pthread support)?

Probably, yes, although it's interesting that .NET has only needed the header file so far -- I guess it's only using the types and macros in that file, and doesn't need any symbols at link time.

dicej added a commit to dicej/runtimelab that referenced this issue May 23, 2024
Some of these changes are borrowed from
dotnet#2569.

Note that I had to manually copy pthread.h from the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi-threads directory to the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi directory as a workaround
until WebAssembly/wasi-libc#501 is addressed.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@SingleAccretion
Copy link

I guess it's only using the types and macros in that file, and doesn't need any symbols at link time.

That's not quite correct. We had to provide the stubs ourselves as well: https://github.com/dotnet/runtimelab/blob/07ff833c2ab3d441e7f77cd195483fea62667039/src/coreclr/nativeaot/Runtime/wasm/PalRedhawkWasm.cpp#L49-L192.

dicej added a commit to dicej/wasi-libc that referenced this issue May 28, 2024
Per WebAssembly#501, this restores the pre-WASI-SDK-22 behavior of including a copy of
pthread.h for all targets -- not just the `*-threads` targets. This helps
minimize the number of preprocessor guards required to port existing POSIX
software to WASI. It also make it easier for projects using WASI-SDK versions
earlier than 22 to upgrade.

Note that this includes the pthread.h header, but no stub function definitions
to link against. We should probably provide the latter as well, but I'll leave
that for a separate PR.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
alexcrichton pushed a commit that referenced this issue May 29, 2024
Per #501, this restores the pre-WASI-SDK-22 behavior of including a copy of
pthread.h for all targets -- not just the `*-threads` targets. This helps
minimize the number of preprocessor guards required to port existing POSIX
software to WASI. It also make it easier for projects using WASI-SDK versions
earlier than 22 to upgrade.

Note that this includes the pthread.h header, but no stub function definitions
to link against. We should probably provide the latter as well, but I'll leave
that for a separate PR.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej added a commit to dicej/runtimelab that referenced this issue May 31, 2024
Some of these changes are borrowed from
dotnet#2569.

Note that I had to manually copy pthread.h from the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi-threads directory to the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi directory as a workaround
until WebAssembly/wasi-libc#501 is addressed.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej added a commit to dicej/runtimelab that referenced this issue May 31, 2024
Some of these changes are borrowed from
dotnet#2569.

Note that I had to manually copy pthread.h from the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi-threads directory to the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi directory as a workaround
until WebAssembly/wasi-libc#501 is addressed.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
jkotas pushed a commit to dotnet/runtimelab that referenced this issue Jun 4, 2024
* tweaks to enable NativeAOT-LLVM on Linux with WASI-SDK 22

Some of these changes are borrowed from
#2569.

Note that I had to manually copy pthread.h from the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi-threads directory to the
wasi-sdk-22/share/wasi-sysroot/include/wasm32-wasi directory as a workaround
until WebAssembly/wasi-libc#501 is addressed.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

* Restrict WASI SDK version check to WASI target

Co-authored-by: yowl <scott.waye@hubse.com>

---------

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Co-authored-by: yowl <scott.waye@hubse.com>
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

No branches or pull requests

4 participants