Bug 210068 – [WPE] Build fixes for musl C library on Linux

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Khem Raj]

Created attachment 395612 [details]
Potential fix

musl is another C library option available for Linux platforms. Currently compiling wpewebkit fails on musl platforms

[Carlos Alberto Lopez Perez]

Comment on attachment 395612 [details]
Potential fix

View in context: https://bugs.webkit.org/attachment.cgi?id=395612&action=review

You need to add changelogs to the patch. Check https://webkit.org/contributing-code/#changelog-files

> b/Source/JavaScriptCore/runtime/MachineContext.h:199
> -#elif OS(FUCHSIA) || defined(__GLIBC__) || defined(__BIONIC__)
> +#elif OS(FUCHSIA) || OS(LINUX) || defined(__BIONIC__)

Can "defined(__BIONIC__)" be true but not "OS(LINUX)"?
If not then we can simplify the condition by removing the check for "defined(__BIONIC__)"

[Khem Raj]

(In reply to Carlos Alberto Lopez Perez from comment #1)
> Comment on attachment 395612 [details]
> Potential fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395612&action=review
> 
> You need to add changelogs to the patch. Check
> https://webkit.org/contributing-code/#changelog-files
> 
> > b/Source/JavaScriptCore/runtime/MachineContext.h:199
> > -#elif OS(FUCHSIA) || defined(__GLIBC__) || defined(__BIONIC__)
> > +#elif OS(FUCHSIA) || OS(LINUX) || defined(__BIONIC__)
> 
> Can "defined(__BIONIC__)" be true but not "OS(LINUX)"?
> If not then we can simplify the condition by removing the check for
> "defined(__BIONIC__)"

if we assume BIONIC is just used for android that runs linux kernel then
maybe, but I dont know where else it is used, if we can be certain thats the only usecase then we can remove this bionic check.

[Zan Dobersek]

Bionic documentation on available defines allows for `__linux__` being optional:
https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/defines.md

[Carlos Alberto Lopez Perez]

(In reply to Zan Dobersek from comment #3)
> Bionic documentation on available defines allows for `__linux__` being
> optional:
> https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/
> defines.md

I do have a different reading of that documentation. I read there that you should use `__linux__` when you not only care about Android but also about other Linux-based systems like GNU/Linux Desktop.

[Khem Raj]

(In reply to Carlos Alberto Lopez Perez from comment #4)
> (In reply to Zan Dobersek from comment #3)
> > Bionic documentation on available defines allows for `__linux__` being
> > optional:
> > https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/
> > defines.md
> 
> I do have a different reading of that documentation. I read there that you
> should use `__linux__` when you not only care about Android but also about
> other Linux-based systems like GNU/Linux Desktop.

my reading is that if you rely on libc interface alone then use __BIONIC__
but if platform is in picture then it is not sufficient. Technically bionic could be used in android-like system which may no be based on Linux kernel.

[Carlos Alberto Lopez Perez]

(In reply to Khem Raj from comment #5)
> Technically bionic could be used in android-like system which may no be based on Linux kernel.

I don't think you can build/use bionic on a non-Linux kernel.

Do you have any pointer/link to any example of this?

[Khem Raj]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Khem Raj from comment #5)
> > Technically bionic could be used in android-like system which may no be based on Linux kernel.
> 
> I don't think you can build/use bionic on a non-Linux kernel.
> 
> Do you have any pointer/link to any example of this?

I am not aware of any usecase myself, but then we have kfreebsd which uses glibc on BSD kernels so its perhaps possible to have bionic on non-linux kernels. it seems wrong to assume BIONIC == __linux__ if we need to know libc interface
although if we think all we need to know if OS here then perhaps using OS(LINUX) is sufficient.

[Yusuke Suzuki]

I personally think that we should explicitly state whether WebKit is built with musl by setting some macro definition like, USE(MUSL), which is passed by CMake.
When building WebKit with musl, we should specify this option explicitly.

The problem of musl is that musl intentionally avoids exposing __musl__ like macro, while musl is not completely compatible to glibc :(

[Carlos Alberto Lopez Perez]

(In reply to Khem Raj from comment #7)
> (In reply to Carlos Alberto Lopez Perez from comment #6)
> > (In reply to Khem Raj from comment #5)
> > > Technically bionic could be used in android-like system which may no be based on Linux kernel.
> > 
> > I don't think you can build/use bionic on a non-Linux kernel.
> > 
> > Do you have any pointer/link to any example of this?
> 
> I am not aware of any usecase myself, but then we have kfreebsd which uses
> glibc on BSD kernels so its perhaps possible to have bionic on non-linux
> kernels. 

Maybe its possible in theory, but not in practice (in the sense you can't do that out of the box).

And regarding GNU/kFreeBSD: The most serious project that I know about of making that a thing its Debian/kFreeBSD. And I know that WebKit doesn't build there.
You can see below all the available architectures Debian build WebKitGTK/WPE related packages for:

https://packages.debian.org/search?keywords=webkit&searchon=names&suite=unstable&section=all
https://packages.debian.org/search?keywords=wpe&searchon=names&suite=unstable&section=all

^^^ And as you can see there are no builds for the Debian kFreeBSD port because it just doesn't build there.

WebKit its a very complex piece of software and there are two many small things that will break in esoteric configurations like those, and there is also very few people interested in fixing this.

> it seems wrong to assume BIONIC == __linux__ if we need to know
> libc interface
> although if we think all we need to know if OS here then perhaps using
> OS(LINUX) is sufficient.

Maybe its wrong from a pure theoretical point of view, but in practice it seems to me that assuming this codepaths should trigger for anything running on a Linux-based kernel its the best thing we can do (IMHO).

[Khem Raj]

Created attachment 396212 [details]
potential patch

[Khem Raj]

Created attachment 396213 [details]
jsc stack adjusts

[Yusuke Suzuki]

Comment on attachment 396213 [details]
jsc stack adjusts

View in context: https://bugs.webkit.org/attachment.cgi?id=396213&action=review

> b/Source/JavaScriptCore/runtime/OptionsList.h:53
> +constexpr unsigned jscMaxPerThreadStack = 5 * MB;
> +constexpr unsigned jscSoftReservedZoneSize = 128 * KB;
> +constexpr unsigned jscReservedZoneSize = 64 * KB;
> +#else
> +constexpr unsigned jscMaxPerThreadStack = 80 * KB;
> +constexpr unsigned jscSoftReservedZoneSize = 32 * KB;
> +constexpr unsigned jscReservedZoneSize = 16 * KB;

This is not correct. Darwin libc should have 5MB stack etc. Please reduce the stack size only when using musl.

[Adrian Perez]

(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 396213 [details]
> jsc stack adjusts
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396213&action=review
> 
> > b/Source/JavaScriptCore/runtime/OptionsList.h:53
> > +constexpr unsigned jscMaxPerThreadStack = 5 * MB;
> > +constexpr unsigned jscSoftReservedZoneSize = 128 * KB;
> > +constexpr unsigned jscReservedZoneSize = 64 * KB;
> > +#else
> > +constexpr unsigned jscMaxPerThreadStack = 80 * KB;
> > +constexpr unsigned jscSoftReservedZoneSize = 32 * KB;
> > +constexpr unsigned jscReservedZoneSize = 16 * KB;
> 
> This is not correct. Darwin libc should have 5MB stack etc. Please reduce
> the stack size only when using musl.

Are the stack size adjustments still needed after the patch for
bug #208223 has landed? It looks to me that setting a known-to-work
value for DEFAULT_THREAD_STACK_SIZE_IN_KB when building with Musl
should be enough 🤔️

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #9)
> (In reply to Khem Raj from comment #7)
> > (In reply to Carlos Alberto Lopez Perez from comment #6)
> > > (In reply to Khem Raj from comment #5)
> > > > Technically bionic could be used in android-like system which may no be based on Linux kernel.
> > > 
> > > I don't think you can build/use bionic on a non-Linux kernel.
> > > 
> > > Do you have any pointer/link to any example of this?
> > 
> > I am not aware of any usecase myself, but then we have kfreebsd which uses
> > glibc on BSD kernels so its perhaps possible to have bionic on non-linux
> > kernels. 
> 
> Maybe its possible in theory, but not in practice (in the sense you can't do
> that out of the box).
> 
> And regarding GNU/kFreeBSD: The most serious project that I know about of
> making that a thing its Debian/kFreeBSD. And I know that WebKit doesn't
> build there.
> You can see below all the available architectures Debian build WebKitGTK/WPE
> related packages for:
> 
> https://packages.debian.org/
> search?keywords=webkit&searchon=names&suite=unstable&section=all
> https://packages.debian.org/
> search?keywords=wpe&searchon=names&suite=unstable&section=all
> 
> ^^^ And as you can see there are no builds for the Debian kFreeBSD port
> because it just doesn't build there.

On the other hand, the FreeBSD ports collection has WebKitGTK 🤷️
 https://svnweb.freebsd.org/ports/head/www/webkit2-gtk3/

> WebKit its a very complex piece of software and there are two many small
> things that will break in esoteric configurations like those, and there is
> also very few people interested in fixing this.
> 
> > it seems wrong to assume BIONIC == __linux__ if we need to know
> > libc interface
> > although if we think all we need to know if OS here then perhaps using
> > OS(LINUX) is sufficient.
> 
> Maybe its wrong from a pure theoretical point of view, but in practice it
> seems to me that assuming this codepaths should trigger for anything running
> on a Linux-based kernel its the best thing we can do (IMHO).

I agree that in practice it seems reasonable to use OS(LINUX) here. If later
somebody who is interested in making things work in a different configuration
not covered by that finds a way, they can propose a patch and we can revisit
this issue.

[Pablo Saavedra]

Comment on attachment 396213 [details]
jsc stack adjusts

View in context: https://bugs.webkit.org/attachment.cgi?id=396213&action=review

>>> b/Source/JavaScriptCore/runtime/OptionsList.h:53
>>> +constexpr unsigned jscReservedZoneSize = 16 * KB;
>> 
>> This is not correct. Darwin libc should have 5MB stack etc. Please reduce the stack size only when using musl.
> 
> Are the stack size adjustments still needed after the patch for
> bug #208223 has landed? It looks to me that setting a known-to-work
> value for DEFAULT_THREAD_STACK_SIZE_IN_KB when building with Musl
> should be enough 🤔️

Since Musl doesn't expose any __MUSL__ definition I dont see other alternative rather set a cmake flag -DUSE_MUSL

[Pablo Saavedra]

Comment on attachment 396213 [details]
jsc stack adjusts

View in context: https://bugs.webkit.org/attachment.cgi?id=396213&action=review

>>>> b/Source/JavaScriptCore/runtime/OptionsList.h:53
>>>> +constexpr unsigned jscReservedZoneSize = 16 * KB;
>>> 
>>> This is not correct. Darwin libc should have 5MB stack etc. Please reduce the stack size only when using musl.
>> 
>> Are the stack size adjustments still needed after the patch for
>> bug #208223 has landed? It looks to me that setting a known-to-work
>> value for DEFAULT_THREAD_STACK_SIZE_IN_KB when building with Musl
>> should be enough 🤔️
> 
> Since Musl doesn't expose any __MUSL__ definition I dont see other alternative rather set a cmake flag -DUSE_MUSL

Could be enough are reverse logic?

```
#if OS(LINUX) && !defined(__GLIBC__)
constexpr unsigned jscMaxPerThreadStack = 80 * KB;
constexpr unsigned jscSoftReservedZoneSize = 32 * KB;
constexpr unsigned jscReservedZoneSize = 16 * KB;
#else
constexpr unsigned jscMaxPerThreadStack = 5 * MB;
constexpr unsigned jscSoftReservedZoneSize = 128 * KB;
constexpr unsigned jscReservedZoneSize = 64 * KB;
#endif
```

[Khem Raj]

(In reply to Adrian Perez from comment #13)
> (In reply to Yusuke Suzuki from comment #12)
> > Comment on attachment 396213 [details]
> > jsc stack adjusts
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396213&action=review
> > 
> > > b/Source/JavaScriptCore/runtime/OptionsList.h:53
> > > +constexpr unsigned jscMaxPerThreadStack = 5 * MB;
> > > +constexpr unsigned jscSoftReservedZoneSize = 128 * KB;
> > > +constexpr unsigned jscReservedZoneSize = 64 * KB;
> > > +#else
> > > +constexpr unsigned jscMaxPerThreadStack = 80 * KB;
> > > +constexpr unsigned jscSoftReservedZoneSize = 32 * KB;
> > > +constexpr unsigned jscReservedZoneSize = 16 * KB;
> > 
> > This is not correct. Darwin libc should have 5MB stack etc. Please reduce
> > the stack size only when using musl.
> 
> Are the stack size adjustments still needed after the patch for
> bug #208223 has landed? It looks to me that setting a known-to-work
> value for DEFAULT_THREAD_STACK_SIZE_IN_KB when building with Musl
> should be enough 🤔️

unfortunately, that does not help completely.

[Khem Raj]

Created attachment 425133 [details]
potential fix

[Yusuke Suzuki]

Comment on attachment 425133 [details]
potential fix

View in context: https://bugs.webkit.org/attachment.cgi?id=425133&action=review

> Source/JavaScriptCore/runtime/OptionsList.h:74
> +#if OS(LINUX) && !defined(__GLIBC__)

Let's add __BIONIC__ too.

> Source/WTF/wtf/Threading.h:63
> +#if OS(LINUX) && !defined(__GLIBC__)

Ditto.

[Khem Raj]

Created attachment 425163 [details]
potential fix

[Khem Raj]

Created attachment 425218 [details]
potential fix 3

[Yusuke Suzuki]

Comment on attachment 425218 [details]
potential fix 3

View in context: https://bugs.webkit.org/attachment.cgi?id=425218&action=review

> Source/WTF/wtf/Threading.h:65
> +#if OS(LINUX) && !defined(__BIONIC__) && !defined(__GLIBC__)
> +#define DEFAULT_THREAD_STACK_SIZE_IN_KB 128
> +#endif

DEFAULT_THREAD_STACK_SIZE_IN_KB is user-configuration. We should not override in our code.
If we need to modify default stack size for that platform, we need to modify Threading.cpp's

static Optional<size_t> stackSize(ThreadType threadType)

function.

[Khem Raj]

Created attachment 425302 [details]
potential fix 4

[Carlos Alberto Lopez Perez]

Comment on attachment 425302 [details]
potential fix 4

Patch is looking good to me.
Not sure if Yusuke wants to give to it the final r+ or someone else wants to comment on it.

[Carlos Alberto Lopez Perez]

Comment on attachment 425302 [details]
potential fix 4

Ok! let's land it

[EWS]

Committed r275670 (236306@main): <https://commits.webkit.org/236306@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425302 [details].

[Yusuke Suzuki]

Looks good to me too!

[Nina Kolesa]

I believe this shouldn't have been merged, the values make jsc rather unstable for complicated javascript and there is no way to tweak them at build time - and as far as i can tell, since was merged a while ago https://bugs.webkit.org/show_bug.cgi?id=208223 it's also unnecessary (the right thing to do is defining DEFAULT_THREAD_STACK_SIZE_IN_KB to some larger value like 2048 or whatever)

the contents of this patch look awfully similar to my patch that i added in my distribution a while back - https://github.com/void-linux/void-packages/blob/5c6495c8acda3463a487b931936823ea25cb1bd7/srcpkgs/webkit2gtk/patches/fix-musl-javascriptcore.patch#L56 i never bothered to upstream it since i never liked this workaround

i'm actually looking into fixing this properly now (currently test-building a tweaked default)

[Nina Kolesa]

of course, as was mentioned above, we shouldn't be tweaking that macro within webkit (rather, the user should change it), for proper fix within webkit i'm not sure (but changing the jsc defaults to small values is IMO not the way to go, as it won't function correctly)

maybe we could set the small default values only if the default stack size has not been tweaked or if it was tweaked to an intentionally small value?

[Nina Kolesa]

some findings:

- upping jscSoftReservedZoneSize beyond 64KB will crash any javascript under musl, regardless of thread stack size
- the Threading.cpp platform changes as they are right now effectively disable being able to set thread stack size via the user macro (as the hardcoded value always returns first)

[Nina Kolesa]

Followup: https://bugs.webkit.org/show_bug.cgi?id=225099

[Adrian Perez]

*** Bug 187485 has been marked as a duplicate of this bug. ***