druntime: fix on x86/arm musl

[sertonix]

tested on alpine linux x86 and armv7

code shrink suggestions welcome! I am not that familiar with dlang

Fixes #21800

[dlang-bot]

Thanks for your pull request and interest in making D better, @sertonix! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21249"

[thewilsonator]

Looks fine to me, but I'll let someone who actually knows something about this a chance to chime in. cc @Geod24

[sertonix]

Is this still stuck in review?

[ibuclaw]

Is this still stuck in review?

Do you have a reference to the headers? It might help.

[sertonix]

Do you have a reference to the headers?

Sure, musl sources for reference (I hope I didn't forgot any important ones):

• struct stat on arm https://git.musl-libc.org/cgit/musl/tree/arch/arm/bits/stat.h
• struct stat on x86 https://git.musl-libc.org/cgit/musl/tree/arch/i386/bits/stat.h
• struct stat on mips https://git.musl-libc.org/cgit/musl/tree/arch/mips/bits/stat.h
• struct timespec (commit includes some explainations) https://git.musl-libc.org/cgit/musl/commit/include/alltypes.h.in?id=9b2921bea1d5017832e1b45d1fd64220047a9802

[kaniini]

What is the status on this? We would like to enable D on 32-bit archs in Alpine, but are blocked by this.

[the-horo]

The changes work for me and I've tested building gdc-11 through gdc-15 on x86 musl.

---

My only concern is that the D bindings still contain the time32 function declarations

dmd/druntime/src/core/sys/posix/sys/time.d

Lines 92 to 95 in c397436

	pragma(mangle, muslRedirTime64Mangle!("gettimeofday", "__gettimeofday_time64"))
	int gettimeofday(timeval*, void*);
	pragma(mangle, muslRedirTime64Mangle!("utimes", "__utimes_time64"))
	int utimes(const scope char*, ref const(timeval)[2]);

which makes it possible for someone to build druntime/phobos with time32 functions while using the time64 structs. This wouldn't be necessarily broken as the structs are meant to be ABI compatible, however, this can still lead to runtime errors because a consumer of the function wouldn't be aware that the 64-bit fields wouldn't contain anything meaningful.

For example, the struct stat_t on x86 musl now looks like:

struct stat_t
{
    dev_t       st_dev;
    ushort      __pad1;
    uint        __st_ino;
    mode_t      st_mode;
    nlink_t     st_nlink;
    uid_t       st_uid;
    gid_t       st_gid;
    dev_t       st_rdev;
    ushort      __pad2;
    off_t       st_size;
    blksize_t   st_blksize;
    blkcnt_t    st_blocks;
	c_long      __st_atime;
	c_long      __st_atimensec;
	c_long      __st_mtime;
	c_long      __st_mtimensec;
	c_long      __st_ctime;
	c_long      __st_ctimensec;
    ino_t       st_ino;
	timespec    st_atim;
	timespec    st_mtim;
	timespec    st_ctim;
	extern(D) @safe @property inout pure nothrow
	{
	    ref inout(time_t) st_atime() return { return st_atim.tv_sec; }
	    ref inout(time_t) st_mtime() return { return st_mtim.tv_sec; }
	    ref inout(time_t) st_ctime() return { return st_ctim.tv_sec; }
	}
}

and the 32bit variant was supposed to look like:

struct stat_t
{
    dev_t       st_dev;
    ushort      __pad1;
    uint        __st_ino;
    mode_t      st_mode;
    nlink_t     st_nlink;
    uid_t       st_uid;
    gid_t       st_gid;
    dev_t       st_rdev;
    ushort      __pad2;
    off_t       st_size;
    blksize_t   st_blksize;
    blkcnt_t    st_blocks;
	time_t      st_atime;
	ulong_t     st_atimensec;
	time_t      st_mtime;
	ulong_t     st_mtimensec;
	time_t      st_ctime;
	ulong_t     st_ctimensec;
   ino_t       st_ino;
}

The issue here is that the st_atime symbol now resolves to different members. Then, consumers like phobos:
https://github.com/dlang/phobos/blob/24711b9c41aae447ff7079017bbe2d1416248fea/std/file.d#L1224-L1243, with this PR applied, would work correctly with time64 but they would be accessing uninitialized memory with the time32 function declarations.

Solving this would require to condition the new symbols on the value of muslRedirTime64:

dmd/druntime/src/core/stdc/config.d

Lines 655 to 661 in 670f8c6

	version (CRuntime_Musl_Pre_Time64)
	enum muslRedirTime64 = false;
	else
	{
	// time_t was defined as a C long in older Musl versions.
	enum muslRedirTime64 = (c_long.sizeof == 4);
	}

Given that currently 32-bit musl is broken I think it's fine to accept this PR which fixes 32-bit musl with the default time64 functions and leave 32-bit musl with the opt-in time32 functions for some other time or until somebody opens an issue requesting support for this, which should only happen if trying to use new D compilers on pre-time64 musl versions.

[sertonix]

Given that currently 32-bit musl is broken I think it's fine to accept this PR which fixes 32-bit musl with the default time64 functions and leave 32-bit musl with the opt-in time32 functions for some other time or until somebody opens an issue requesting support for this, which should only happen if trying to use new D compilers on pre-time64 musl versions.

I currently don't have the time and setup to fix that as well. So I would prefer to ignore that in this MR (especially since this only affects old musl versions).

[ibuclaw]

@sertonix WRT acceptance without the current pending PRs being merged. My only comment is that making changes to the existing definitions inline is a bit unwieldy wrt any potential future maintenance.

So my preference would be to hoist out version (CRuntime_Musl) into its own independent definition of each of these structs.

To take stat_t as the unfortunate victim to pick on, this is simpler than the current static condition spaghetti this PR has the definition in:

version (CRuntime_Musl)
{
    // Musl always uses 64 bit off_t
    struct stat_t
    {
        dev_t st_dev;
        int __st_dev_padding;
        c_long __st_ino_truncated;
        mode_t st_mode;
        nlink_t st_nlink;
        uid_t st_uid;
        gid_t st_gid;
        dev_t st_rdev;
        int __st_rdev_padding;
        off_t st_size;
        blksize_t st_blksize;
        blkcnt_t st_blocks;
        private struct __timespec32
        {
            c_long tv_sec;
            c_long tv_nsec;
        }
        __timespec32 __st_atim32;
        __timespec32 __st_mtim32;
        __timespec32 __st_ctim32;
        ino_t st_ino;
        timespec st_atim;
        timespec st_mtim;
        timespec st_ctim;
        extern(D) @safe @property inout pure nothrow
        {
            ref inout(time_t) st_atime() return { return st_atim.tv_sec; }
            ref inout(time_t) st_mtime() return { return st_mtim.tv_sec; }
            ref inout(time_t) st_ctime() return { return st_ctim.tv_sec; }
        }
    }
}
else
{
    struct stat_t
    {
        dev_t       st_dev;
        ushort      __pad1;
        static if (!__USE_FILE_OFFSET64)
        {
            ino_t       st_ino;
        }
        else
        {
            uint        __st_ino;
        }
        mode_t      st_mode;
        nlink_t     st_nlink;
        uid_t       st_uid;
        gid_t       st_gid;
        dev_t       st_rdev;
        ushort      __pad2;
        off_t       st_size;
        blksize_t   st_blksize;
        blkcnt_t    st_blocks;
        static if (_DEFAULT_SOURCE || _XOPEN_SOURCE >= 700) 
        {
            timespec    st_atim;
            timespec    st_mtim;
            timespec    st_ctim;
            extern(D) @safe @property inout pure nothrow
            {
                ref inout(time_t) st_atime() return { return st_atim.tv_sec; }
                ref inout(time_t) st_mtime() return { return st_mtim.tv_sec; }
                ref inout(time_t) st_ctime() return { return st_ctim.tv_sec; }
            }
        }
        else
        {
            time_t      st_atime;
            ulong_t     st_atimensec;
            time_t      st_mtime;
            ulong_t     st_mtimensec;
            time_t      st_ctime;
            ulong_t     st_ctimensec;
        }
        static if (__USE_FILE_OFFSET64)
        {
            ino_t       st_ino;
        }
        else
        {
            c_ulong     __unused4;
            c_ulong     __unused5;
        }
    }
}

[sertonix]

So my preference would be to hoist out version (CRuntime_Musl) into its own independent definition of each of these structs.

If that is limited to timespec and the stat_t that differ I would be fine with that

[ibuclaw]

So my preference would be to hoist out version (CRuntime_Musl) into its own independent definition of each of these structs.

If that is limited to timespec and the stat_t that differ I would be fine with that

That's fine. Its only 32bit platforms affected.

[ibuclaw]

Just a couple of style nits, but otherwise looks OK.

done