Bug 210068 - [WPE] Build fixes for musl C library on Linux
Summary: [WPE] Build fixes for musl C library on Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 187485 (view as bug list)
Depends on: 208223
Blocks: 187485 226244
  Show dependency treegraph
 
Reported: 2020-04-06 13:25 PDT by Khem Raj
Modified: 2021-05-25 15:31 PDT (History)
18 users (show)

See Also:


Attachments
Potential fix (2.65 KB, patch)
2020-04-06 13:25 PDT, Khem Raj
no flags Details | Formatted Diff | Diff
potential patch (2.51 KB, patch)
2020-04-11 22:08 PDT, Khem Raj
no flags Details | Formatted Diff | Diff
jsc stack adjusts (2.13 KB, patch)
2020-04-11 22:08 PDT, Khem Raj
no flags Details | Formatted Diff | Diff
potential fix (7.02 KB, patch)
2021-04-04 14:14 PDT, Khem Raj
no flags Details | Formatted Diff | Diff
potential fix (7.08 KB, patch)
2021-04-05 09:43 PDT, Khem Raj
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
potential fix 3 (7.08 KB, patch)
2021-04-05 15:46 PDT, Khem Raj
no flags Details | Formatted Diff | Diff
potential fix 4 (7.27 KB, patch)
2021-04-06 11:05 PDT, Khem Raj
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Khem Raj 2020-04-06 13:25:50 PDT
Created attachment 395612 [details]
Potential fix

musl is another C library option available for Linux platforms. Currently compiling wpewebkit fails on musl platforms
Comment 1 Carlos Alberto Lopez Perez 2020-04-06 14:46:08 PDT
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__)"
Comment 2 Khem Raj 2020-04-07 08:08:21 PDT
(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.
Comment 3 Zan Dobersek 2020-04-09 00:27:14 PDT
Bionic documentation on available defines allows for `__linux__` being optional:
https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/defines.md
Comment 4 Carlos Alberto Lopez Perez 2020-04-09 04:43:15 PDT
(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.
Comment 5 Khem Raj 2020-04-09 07:28:12 PDT
(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.
Comment 6 Carlos Alberto Lopez Perez 2020-04-09 10:14:21 PDT
(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?
Comment 7 Khem Raj 2020-04-09 13:45:05 PDT
(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.
Comment 8 Yusuke Suzuki 2020-04-09 13:50:48 PDT
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 :(
Comment 9 Carlos Alberto Lopez Perez 2020-04-09 18:47:09 PDT
(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).
Comment 10 Khem Raj 2020-04-11 22:08:05 PDT
Created attachment 396212 [details]
potential patch
Comment 11 Khem Raj 2020-04-11 22:08:49 PDT
Created attachment 396213 [details]
jsc stack adjusts
Comment 12 Yusuke Suzuki 2020-04-14 09:56:00 PDT
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.
Comment 13 Adrian Perez 2020-04-15 08:48:00 PDT
(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 🤔️
Comment 14 Adrian Perez 2020-04-15 09:00:10 PDT
(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.
Comment 15 Pablo Saavedra 2020-06-08 14:18:33 PDT
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
Comment 16 Pablo Saavedra 2020-06-08 14:31:10 PDT
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
```
Comment 17 Khem Raj 2021-04-04 13:54:44 PDT
(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.
Comment 18 Khem Raj 2021-04-04 14:14:07 PDT
Created attachment 425133 [details]
potential fix
Comment 19 Yusuke Suzuki 2021-04-05 00:47:45 PDT
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.
Comment 20 Khem Raj 2021-04-05 09:43:07 PDT
Created attachment 425163 [details]
potential fix
Comment 21 Khem Raj 2021-04-05 15:46:14 PDT
Created attachment 425218 [details]
potential fix 3
Comment 22 Yusuke Suzuki 2021-04-05 17:04:21 PDT
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.
Comment 23 Khem Raj 2021-04-06 11:05:59 PDT
Created attachment 425302 [details]
potential fix 4
Comment 24 Carlos Alberto Lopez Perez 2021-04-06 15:33:45 PDT
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.
Comment 25 Carlos Alberto Lopez Perez 2021-04-08 11:33:37 PDT
Comment on attachment 425302 [details]
potential fix 4

Ok! let's land it
Comment 26 EWS 2021-04-08 12:05:39 PDT
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].
Comment 27 Yusuke Suzuki 2021-04-08 13:50:47 PDT
Looks good to me too!
Comment 28 Daniel Kolesa 2021-04-20 15:38:27 PDT
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)
Comment 29 Daniel Kolesa 2021-04-20 15:41:33 PDT
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?
Comment 30 Daniel Kolesa 2021-04-20 19:22:51 PDT
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)
Comment 31 Daniel Kolesa 2021-04-27 06:36:14 PDT
Followup: https://bugs.webkit.org/show_bug.cgi?id=225099
Comment 32 Adrian Perez 2021-05-25 15:28:09 PDT
*** Bug 187485 has been marked as a duplicate of this bug. ***