[RFC] Initial support for building druntime with musl libc #1997

yshui · 2017-12-17T08:48:46Z

Make druntime buildable with CRuntime_Musl defined instead of CRuntime_Glibc.

dlang-bot · 2017-12-17T08:48:46Z

Thanks for your pull request, @yshui! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

rikkimax · 2017-12-17T09:07:14Z

src/core/sys/posix/signal.d

@@ -544,6 +544,28 @@ version( CRuntime_Glibc )
        void function() sa_restorer;
    }
 }
+version( CRuntime_Musl )


You missed an else.

Causing: src/core/sys/posix/signal.d(686): Error: static assert "Architecture not supported.".

PetarKirov · 2017-12-17T10:15:28Z

@yshui Thanks for looking into adding musl libc support to druntime. It looks like there are many things that need to be changed, which is why I highly recommend splitting your work into many commits. That would allow us merge changes that are ready much faster, so we can focus on the rest.

@wilzbach If I remember correctly, you had a tool to split work into multiple commits / pull-requests automatically?

wilzbach · 2017-12-17T10:21:16Z

This is really cool - any chance we can test this e.g. on CircleCi automatically? :)

@wilzbach If I remember correctly, you had a tool to split work into multiple commits / pull-requests automatically?

Yes, it's a stupid bash script that will simply make a PR for every file.

https://github.com/wilzbach/splitup-prs

PetarKirov · 2017-12-17T10:51:47Z

@yshui I'm now reviewing your changes file by file and I'll get back to you with more comments after 1-2 hours.

This is really cool - any chance we can test this e.g. on CircleCi automatically? :)

@wilzbach very good idea, but I don't think it's easily doable in the short term, since in order to to run the dmd/druntime/phobos tests on a musl libc system, we first need to compile them for which we need either cross-compilation (telling dmd that host != target, something which only LDC and GDC support well) or a good bootstrap process (but I'm not sure how usable are the dmd-cxx branches).

Certainly after we merge these changes we can backport them to GDC/LDC and use them to build DMD.

wilzbach · 2017-12-17T11:02:12Z

good bootstrap process (but I'm not sure how usable are the dmd-cxx branches).

The latest C++ release can still compile master without any work. @ibuclaw's dmd-cxx branch contains a couple of backports, but IIRC druntime doesn't build with them yet.
Anyhow there's @CyberShadow's amazing Digger which does an excellent job at bootstrapping.

ibuclaw · 2017-12-17T11:18:53Z

@ibuclaw's dmd-cxx branch contains a couple of backports, but IIRC druntime doesn't build with them yet.

https://github.com/dlang/dmd/blob/dmd-cxx/travis.sh

I wouldn't have uncommented it if didn't work for me.

Someone should take ownership in looking at the glue/backend in that branch anyway. I have repeatedly said that only the frontend backed by gdc has been rigorously tested, which is backed by semaphore and buildbot ci which checks 6 distinct CPU targets.

joakim-noah · 2017-12-17T19:38:23Z

@yshui, nice work, I'll take a look.

@ZombineDev, never messed with Musl, can it be used on a regular glibc distro too? If so, setting up CI should be easy.

@ibuclaw, I'd be happy to try testing the dmd-cxx branch, but I was under the impression you were going to update the frontend on that branch further, dlang/dmd#7227.

PetarKirov

I did an initial review. My general concerns that I would like you to address are:

Use separate definitions for each supported by musl architecture. For example see my comment about fenv_t.
Add link to the relevant musl source file
New code added by you should follow https://dlang.org/dstyle
Try to fill all empty else version (CRuntime_Musl) { } blocks that you have added.

I haven't been able to review all your changes as they are too many for one pull-request (and especially one commit!), but I hope this should be enough get you going the right direction.

PetarKirov · 2017-12-17T10:25:14Z

src/core/stdc/assert_.d

@@ -63,6 +63,10 @@ else version (CRuntime_Glibc)
    void __assert_fail(const(char)* exp, const(char)* file, uint line, const(char)* func);
    ///
    void __assert_perror_fail(int errnum, const(char)* file, uint line, const(char)* func);
+}
+else version (CRuntime_Musl)


I know that hitting static assert(0) can be annoying at the beginning when you just want see how far you can go, but to get your changes to into merge-able state, you need to fill out all empty else version (CRuntime_Musl) { } blocks that you added.

In this case, looking at include/assert.h (from git.musl-libc.org) and the glibc version above, you need to add:

/*** * Assert failure function in the Musl C library. */ void __assert_fail(const(char)* exp, const(char)* file, uint line, const(char)* func);

Or alternately, just leave empty blocks out of this pull for now, except for those that assert without them. In this case, you could just add a TODO comment here, as this is only needed for the betterC support. I don't know that adding __assert_fail alone does much, as dmd likely only calls __assert.

PetarKirov · 2017-12-17T10:31:08Z

src/core/stdc/fenv.d

@@ -298,6 +298,25 @@ else version( Solaris )

    alias int fexcept_t;
 }
+else version( CRuntime_Musl )


Historically, druntime was written in a different code-style, but for all new code we try to follow https://dlang.org/dstyle.
-> use else version (CRuntime_Musl)

Druntime uses both styles, I'm guessing he just copy-pasted this style from above. No need to nitpick this, can always change it later.

PetarKirov · 2017-12-17T11:25:35Z

src/core/stdc/fenv.d

@@ -298,6 +298,25 @@ else version( Solaris )

    alias int fexcept_t;
 }
+else version( CRuntime_Musl )
+{
+    struct fenv_t {


This is not correct, since fenv_t has architecture dependent implementation. I strongly suggest you follow the GNUFP code above as a good example, in other words:

else version (CRuntime_Musl) { // http://git.musl-libc.org/cgit/musl/tree/arch/i386/bits/fenv.h version (X86) { struct fenv_t { ushort __control_word; ushort __unused1; ushort __status_word; ushort __unused2; ushort __tags; ushort __unused3; uint __eip; ushort __cs_selector; ushort __opcode; uint __data_offset; ushort __data_selector; ushort __unused5; } alias fexcept_t = ushort; } // http://git.musl-libc.org/cgit/musl/tree/arch/x86_64/bits/fenv.h else version (X86_64) { struct fenv_t { ushort __control_word; ushort __unused1; ushort __status_word; ushort __unused2; ushort __tags; ushort __unused3; uint __eip; ushort __cs_selector; ushort __opcode; uint __data_offset; ushort __data_selector; ushort __unused5; uint __mxcsr; } alias fexcept_t = ushort; } // http://git.musl-libc.org/cgit/musl/tree/arch/mipsn32/bits/fenv.h else version (MIPS32) { struct fenv_t { uint __cw; } alias fexcept_t = ushort; } // http://git.musl-libc.org/cgit/musl/tree/arch/mips64/bits/fenv.h else version (MIPS64) { struct fenv_t { uint __cw; } alias fexcept_t = ushort; } // http://git.musl-libc.org/cgit/musl/tree/arch/aarch64/bits/fenv.h else version (AArch64) { struct fenv_t { uint __fpcr; uint __fpsr; } alias fexcept_t = uint; } // http://git.musl-libc.org/cgit/musl/tree/arch/arm/bits/fenv.h else version (ARM) { struct fenv_t { // FIXME: musl defines this as `unsigned long` - check if // this actually is 32-bits in size uint __cw; } // FIXME: musl defines this as `unsigned long` - check if // this actually is 32-bits in size alias fexcept_t = uint; } // http://git.musl-libc.org/cgit/musl/tree/arch/powerpc64/bits/fenv.h else version (PPC64) { alias fenv_t = double; alias fexcept_t = uint; } // http://git.musl-libc.org/cgit/musl/tree/arch/s390x/bits/fenv.h else version (SystemZ) { // Note: different from the glibc implementation: // https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/s390/fpu/bits/fenv.h alias fenv_t = uint; alias fexcept_t = uint; } else { static assert(0, "Unimplemented architecture support for musl libc"); } }

While Petar is right that architecture-dependent code should be versioned like this, you don't have to add any arch that you have not tried, so just adding the version(X86) guard and maybe version(X86_64) should be enough.

PetarKirov · 2017-12-17T11:29:58Z

src/core/stdc/fenv.d

@@ -623,6 +642,9 @@ else version( Solaris )
    ///
    enum FE_DFL_ENV = &__fenv_def_env;
 }
+else version( CRuntime_Musl )
+{


// http://git.musl-libc.org/cgit/musl/tree/arch/generic/bits/fenv.h#n10 // (implemented the same way for all architectures) enum FE_DFL_ENV = cast(fenv_t*)(-1);

PetarKirov · 2017-12-17T11:35:37Z

src/core/stdc/locale.d

@@ -218,6 +218,9 @@ else version(Solaris)
    ///
    enum LC_ALL         = 6;
 }
+else version(CRuntime_Musl)
+{


else version (CRuntime_Musl) { // http://git.musl-libc.org/cgit/musl/tree/include/locale.h /// enum LC_CTYPE = 0; /// enum LC_NUMERIC = 1; /// enum LC_TIME = 2; /// enum LC_COLLATE = 3; /// enum LC_MONETARY = 4; /// enum LC_MESSAGES = 5; /// enum LC_ALL = 6; }

PetarKirov · 2017-12-17T12:38:55Z

src/core/sys/posix/time.d

+    int clock_gettime(clockid_t, timespec*);
+    int clock_settime(clockid_t, in timespec*);
+    int clock_nanosleep(clockid_t, int, in timespec*, timespec*);
+}


else version (CRuntime_Musl) { // http://git.musl-libc.org/cgit/musl/tree/include/time.h alias clockid_t = int; alias timer_t = void*; struct itimerspec { timespec it_interval; timespec it_value; } enum TIMER_ABSTIME = 1; enum CLOCK_REALTIME = 0; enum CLOCK_MONOTONIC = 1; enum CLOCK_PROCESS_CPUTIME_ID = 2; enum CLOCK_THREAD_CPUTIME_ID = 3; enum CLOCK_MONOTONIC_RAW = 4; enum CLOCK_REALTIME_COARSE = 5; enum CLOCK_MONOTONIC_COARSE = 6; enum CLOCK_BOOTTIME = 7; enum CLOCK_REALTIME_ALARM = 8; enum CLOCK_BOOTTIME_ALARM = 9; enum CLOCK_SGI_CYCLE = 10; enum CLOCK_TAI = 11; int nanosleep(in timespec*, timespec*); int clock_getres(clockid_t, timespec*); int clock_gettime(clockid_t, timespec*); int clock_settime(clockid_t, in timespec*); int clock_nanosleep(clockid_t, int, in timespec*, timespec*); int clock_getcpuclockid(pid_t, clockid_t *); int timer_create(clockid_t, sigevent*, timer_t*); int timer_delete(timer_t); int timer_gettime(timer_t, itimerspec*); int timer_settime(timer_t, int, in itimerspec*, itimerspec*); int timer_getoverrun(timer_t); }

PetarKirov · 2017-12-17T12:44:14Z

src/core/sys/posix/time.d

+}
+else version (CRuntime_Musl)
+{
+
 }


else version (CRuntime_Musl) { char* asctime_r(in tm*, char*); char* ctime_r(in time_t*, char*); tm* gmtime_r(in time_t*, tm*); tm* localtime_r(in time_t*, tm*); }

PetarKirov · 2017-12-17T12:58:51Z

src/core/sys/posix/sys/wait.d

+    extern (D) bool WIFSTOPPED( int status ) { return cast(short)(((status&0xffff)*0x10001)>>8) > 0x7f00; }
+    extern (D) int  WTERMSIG( int status ) { return status & 0x7F; }
+    alias WEXITSTATUS WSTOPSIG;
+}


else version (CRuntime_Musl) { // http://git.musl-libc.org/cgit/musl/tree/include/sys/wait.h enum WNOHANG = 1; enum WUNTRACED = 2; extern (D) int WEXITSTATUS(int status) { return (status & 0xFF00) >> 8; } extern (D) int WIFCONTINUED(int status) { return status == 0xffff; } extern (D) bool WIFEXITED(int status) { return WTERMSIG(status) == 0; } extern (D) bool WIFSIGNALED(int status) { return (status & 0xffff) -1U < 0xffU; } extern (D) bool WIFSTOPPED(int status) { return (((status & 0xffff) * 0x10001) >> 8) > 0x7f00; } extern (D) int WTERMSIG(int status) { return status & 0x7F; } alias WSTOPSIG = WEXITSTATUS; }

PetarKirov · 2017-12-17T13:11:31Z

src/core/sys/posix/sys/uio.d

+        void* iov_base;
+        uint  iov_len;
+    }
+}


else version (CRuntime_Musl) { // http://git.musl-libc.org/cgit/musl/tree/include/alltypes.h.in // http://git.musl-libc.org/cgit/musl/tree/include/sys/uio.h struct iovec { void* iov_base; size_t iov_len; } ssize_t readv(int fd, in iovec* iov, int iovcnt); ssize_t writev(int fd, in iovec* iov, int iovcnt); ssize_t preadv(int fd, in iovec* iov, int iovcnt, off_t offset); ssize_t pwritev(int fd, in iovec* iov, int iovcnt, off_t offset); alias preadv64 = preadv; alias pwritev64 = pwritev; ssize_t process_vm_writev(pid_t pid, in iovec* local_iov, c_long liovcnt, in iovec* remote_iov, c_long riovcnt, c_long flags); ssize_t process_vm_readv(pid_t pid, in iovec* local_iov, c_long liovcnt, in iovec* remote_iov, c_long riovcnt, c_long flags); }

PetarKirov · 2017-12-17T19:33:28Z

src/core/sys/posix/sys/types.d

@@ -604,6 +637,37 @@ version (CRuntime_Glibc)

    alias c_ulong pthread_t;
 }
+else version( CRuntime_Musl )
+{
+    union pthread_attr_t


Again, please separate the definitions per architecture. For reference:
http://git.musl-libc.org/cgit/musl/tree/arch/i386/bits/alltypes.h.in
http://git.musl-libc.org/cgit/musl/tree/arch/x86_64/bits/alltypes.h.in
http://git.musl-libc.org/cgit/musl/tree/arch/arm/bits/alltypes.h.in
http://git.musl-libc.org/cgit/musl/tree/arch/aarch64/bits/alltypes.h.in
http://git.musl-libc.org/cgit/musl/tree/arch/mipsn32/bits/alltypes.h.in
http://git.musl-libc.org/cgit/musl/tree/arch/mips64/bits/alltypes.h.in
...

As I said above, version the architecture, but for this pull only the ones you tested are enough. We welcome declarations for other arches, but you don't have to add them if you don't want.

PetarKirov · 2017-12-17T19:48:51Z

@ZombineDev, never messed with Musl, can it be used on a regular glibc distro too? If so, setting up CI should be easy.

@joakim-noah I wasn't sure too, but looking at https://github.com/emk/rust-musl-builder it seems it should be doable.

wilzbach · 2017-12-17T19:56:00Z

@ZombineDev, never messed with Musl, can it be used on a regular glibc distro too? If so, setting up CI should be easy.

Yes, it actually looks pretty straight-forward. They even provide a gcc wrapper and just statically link it in:

https://wiki.musl-libc.org/getting-started.html#Using-the-musl-gcc-wrapper

(I haven't played with musl either)

joakim-noah

Went over this for basic formatting errors and to make sure everything is scoped to version(CRuntime_Musl). Where you edited non-Musl code, I checked that and found some errors.

I think this is fine to all be in one pull, but you may want to split it into 2-3 pulls to make it easier for you and the reviewers. As for adding source links, that's up to you and would be nice to have, but I don't think we require it. Do we, @MartinNowak or @klickverbot?

joakim-noah · 2017-12-17T21:31:54Z

src/core/stdc/assert_.d

@@ -63,6 +63,10 @@ else version (CRuntime_Glibc)
    void __assert_fail(const(char)* exp, const(char)* file, uint line, const(char)* func);
    ///
    void __assert_perror_fail(int errnum, const(char)* file, uint line, const(char)* func);
+}
+else version (CRuntime_Musl)


Or alternately, just leave empty blocks out of this pull for now, except for those that assert without them. In this case, you could just add a TODO comment here, as this is only needed for the betterC support. I don't know that adding __assert_fail alone does much, as dmd likely only calls __assert.

joakim-noah · 2017-12-17T21:35:31Z

src/core/stdc/fenv.d

@@ -298,6 +298,25 @@ else version( Solaris )

    alias int fexcept_t;
 }
+else version( CRuntime_Musl )


Druntime uses both styles, I'm guessing he just copy-pasted this style from above. No need to nitpick this, can always change it later.

joakim-noah · 2017-12-17T21:37:40Z

src/core/stdc/fenv.d

@@ -298,6 +298,25 @@ else version( Solaris )

    alias int fexcept_t;
 }
+else version( CRuntime_Musl )
+{
+    struct fenv_t {


While Petar is right that architecture-dependent code should be versioned like this, you don't have to add any arch that you have not tried, so just adding the version(X86) guard and maybe version(X86_64) should be enough.

joakim-noah · 2017-12-17T21:40:25Z

src/core/stdc/stdio.d

+        TMP_MAX      = 238328,
+        ///
+        L_tmpnam     = 20
+    }


I'm guessing he just copy-pasted one of the blocks above. Please don't submit code like this that is just copy-pasted from above without being checked.

joakim-noah · 2017-12-17T21:53:59Z

src/core/sys/posix/sys/socket.d

@@ -1368,7 +1368,7 @@ else version( CRuntime_Bionic )

        enum
        {
-            SOL_SOCKET      = 1
+           SOL_SOCKET      = 1


Mistake, take this whitespace deletion out, as it was correct before.

joakim-noah · 2017-12-17T21:55:45Z

src/core/sys/posix/sys/types.d

@@ -604,6 +637,37 @@ version (CRuntime_Glibc)

    alias c_ulong pthread_t;
 }
+else version( CRuntime_Musl )
+{
+    union pthread_attr_t


As I said above, version the architecture, but for this pull only the ones you tested are enough. We welcome declarations for other arches, but you don't have to add them if you don't want.

joakim-noah · 2017-12-17T22:08:43Z

src/core/time.d

@@ -292,7 +292,7 @@ else version(Darwin) enum ClockType
    precise = 3,
    second = 6,
 }
-else version(linux) enum ClockType
+else version(CRuntime_Glibc) enum ClockType


This is going to break this module for Bionic and the tests below for Musl. I see no reason to change this, even if these constants aren't defined for you.

joakim-noah · 2017-12-17T22:19:29Z

src/core/time.d

+            version(CRuntime_Glibc) {
+                case processCPUTime: return CLOCK_PROCESS_CPUTIME_ID;
+                case threadCPUTime: return CLOCK_THREAD_CPUTIME_ID;
+            }


These won't be defined for Bionic anymore, and they appear to be defined for Musl?

joakim-noah · 2018-01-30T05:59:09Z

Do you plan on resurrecting the parts of this pull that haven't been submitted elsewhere? If so, maybe you can just reopen this pull with that remaining patch.

Initial support for building druntime with musl libc

0164493

Make druntime buildable with CRuntime_Musl defined instead of CRuntime_Glibc.

rikkimax suggested changes Dec 17, 2017

View reviewed changes

PetarKirov suggested changes Dec 17, 2017

View reviewed changes

joakim-noah suggested changes Dec 17, 2017

View reviewed changes

yshui mentioned this pull request Dec 26, 2017

Add musl libc definitions to: src_core_thread #2007

Merged

yshui closed this Dec 26, 2017

yshui mentioned this pull request Jan 28, 2018

Add musl libc definitions to core/stdc/* #2066

Merged

[RFC] Initial support for building druntime with musl libc #1997

[RFC] Initial support for building druntime with musl libc #1997

Conversation

yshui commented Dec 17, 2017

dlang-bot commented Dec 17, 2017

Bugzilla references

Choose a reason for hiding this comment

PetarKirov commented Dec 17, 2017 • edited Loading

wilzbach commented Dec 17, 2017

PetarKirov commented Dec 17, 2017 • edited Loading

wilzbach commented Dec 17, 2017

ibuclaw commented Dec 17, 2017

joakim-noah commented Dec 17, 2017

PetarKirov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov commented Dec 17, 2017

wilzbach commented Dec 17, 2017

joakim-noah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah commented Jan 30, 2018

PetarKirov commented Dec 17, 2017 •

edited

Loading

PetarKirov commented Dec 17, 2017 •

edited

Loading

PetarKirov left a comment •

edited

Loading

joakim-noah Dec 17, 2017 •

edited

Loading