Summary: | [WPE] Build fixes for musl C library on Linux | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Khem Raj <raj.khem> | ||||||||||||||||
Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aperez, beb5pft5lz, benjamin, bugs-noreply, cdumez, clopez, cmarcelo, dkolesa, ews-watchlist, keith_miller, mark.lam, msaboff, pnormand, psaavedra, saam, tzagallo, ysuzuki, zan | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=187485 | ||||||||||||||||||
Bug Depends on: | 208223 | ||||||||||||||||||
Bug Blocks: | 187485, 226244 | ||||||||||||||||||
Attachments: |
|
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__)" (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. Bionic documentation on available defines allows for `__linux__` being optional: https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/defines.md (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. (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. (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? (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. 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 :( (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§ion=all https://packages.debian.org/search?keywords=wpe&searchon=names&suite=unstable§ion=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). Created attachment 396212 [details]
potential patch
Created attachment 396213 [details]
jsc stack adjusts
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. (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 🤔️ (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§ion=all > https://packages.debian.org/ > search?keywords=wpe&searchon=names&suite=unstable§ion=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 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 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 ``` (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. Created attachment 425133 [details]
potential fix
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. Created attachment 425163 [details]
potential fix
Created attachment 425218 [details]
potential fix 3
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. Created attachment 425302 [details]
potential fix 4
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 on attachment 425302 [details]
potential fix 4
Ok! let's land it
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]. Looks good to me too! 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) 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? 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) *** Bug 187485 has been marked as a duplicate of this bug. *** |
Created attachment 395612 [details] Potential fix musl is another C library option available for Linux platforms. Currently compiling wpewebkit fails on musl platforms