CRuntime_Musl: fixes for time64 #3383

omerfirmak · 2021-02-26T21:31:38Z

The original PR (#3275) missed quite a few spots and conversions,
which led to the build on Alpine Linux failing with Aithmetic Exception
on core.time module constructor.
Links to the two offending commits are included.
For further issues / investigation, search for 'time64' in the git repository.

dlang-bot · 2021-02-26T21:31:39Z

Thanks for your pull request and interest in making D better, @omerfirmak! 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 "dmd-cxx + druntime#3383"

changelog/musl-32bits.dd

ibuclaw · 2021-02-27T05:15:27Z

changelog/musl-32bits.dd

+This change should only affect packagers for Musl-based systems who support
+32 bits architectures (64 bits architectures already use 64 bits `time_t`),
+who now need to define `CRuntime_Musl_Pre_Time64` both when building
+druntime / Phobos and in the default configuration, if the linked Musl is < 1.2.0.


Is there a configure test that could be used by gdc for its build system, so that the burden isn't shouldered to the users/client code?

In particular, when running the testsuite.

ibuclaw · 2021-02-27T05:17:23Z

src/core/stdc/time.d

    public import core.sys.windows.stdc.time;
+    // This enum is defined only for Posix, this file is the only one
+    // needing it in `core.stdc`.
+    private enum CRuntime_Musl_Needs_Time64_Compat_Layer = false;


Perhaps all function declarations present here should be moved to core.sys.<platform>.stdc.time then.

ibuclaw · 2021-02-27T05:19:20Z

src/core/sys/linux/sys/socket.d

+{
+    enum SO_TIMESTAMP = 63;
+    enum SO_TIMESTAMPNS = 64;
+    enum SO_TIMESTAMPING = 65;


Wait, so now there is backwards compatibility with Musl, but not older Linux kernels?

We probably should just ditch backward compatibility for Musl. And that's a mistake, too.

src/core/stdc/time.d

As explained in the comment, `time_t` on Musl is now always 64 bits, but used to be 32 bits on 32 bits systems. CRuntime_Musl: More fixes for time64 The original PR (dlang#3275) missed quite a few spots and conversions, which led to the build on Alpine Linux failing with Aithmetic Exception on core.time module constructor. Links to the two offending commits are included. For further issues / investigation, search for 'time64' in the git repository. Co-Authored-By: Ömer Faruk IRMAK <omerfirmak@gmail.com>

RazvanN7 · 2021-03-03T11:26:54Z

@Geod24 What is your stance on this?

RazvanN7 · 2021-03-12T09:56:23Z

@ibuclaw Is it ok to merge this?

ibuclaw

We should avoid using static if inside core.stdc.time, otherwise what was the point of splitting the module into core.sys.posix.stdc.time and windows.stdc.time in the first place.

There's no apparent backwards compatibility for older Linux versions (people are still occasionally running gdc testsuite on centos with 2.6.xx), and yet musl is given that luxury.

A single core.sys.posix.config option might be the best way to go, rather than the mixed version/enum approach.

Geod24 · 2021-03-12T12:25:12Z

@RazvanN7 : This needs a bit more love, I just didn't have much time for it recently. Will give it a shot this WE.

RazvanN7 · 2022-04-28T13:17:15Z

@Geod24 any progress on this?

On 32-bit arches, gdc fails to compile any software with an arithmetic error. This is a known upstream bug, there is a patch but it is not yet merged and requires some tinkering to apply cleanly for GCC 12.X. See: * dlang/druntime#3383 * https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/36469#note_248077

nmeum · 2022-08-12T13:06:45Z

Hello, Alpine Linux contributor here 👋 We just upgraded our Alpine GCC package from GCC 11 to GCC 12.1 but due to the Aithmetic Exception described in this PR we were not able to get GDC to work on our 32-bit architectures (armhf, armv7, x86) and hence had to disable it.

I would appreciate it if someone finds the time to finish work on this PR as this would allow us to enable GDC again for these architectures. If this PR gets merged it would also be nice if it could be backported to the GCC 12.1 release branch (releases/gcc-12.1.0), if possible.

thewilsonator · 2022-08-12T13:13:50Z

@nmeum this PR needs to be retargeted to dlang/dmd as this repo is effectively archive only (it was merged into dlang/dmd to reduce hassle of multiple git modules). It can then be picked up by @ibuclaw (ping) to be merged into GDC.

thewilsonator · 2022-08-12T13:15:49Z

Oh I just realised this PR is targeted to the dmd-cxx branch.
I'm not sure how that factors into the merging of the druntime repo in to the compiler repo.

ibuclaw · 2022-08-13T09:29:53Z

Hello, Alpine Linux contributor here wave We just upgraded our Alpine GCC package from GCC 11 to GCC 12.1 but due to the Aithmetic Exception described in this PR we were not able to get GDC to work on our 32-bit architectures (armhf, armv7, x86) and hence had to disable it.

You have a huge list of custom patches to gcc, you can simply include this as another.

https://gitlab.alpinelinux.org/alpine/aports/-/tree/master/main/gcc

I would appreciate it if someone finds the time to finish work on this PR as this would allow us to enable GDC again for these architectures. If this PR gets merged it would also be nice if it could be backported to the GCC 12.1 release branch (releases/gcc-12.1.0), if possible.

GCC release tags are immutable, you have a week to sort it out until 12.2 is cut, otherwise wait until 12.3.

nmeum · 2022-08-13T12:58:59Z

You have a huge list of custom patches to gcc, you can simply include this as another.

I am well aware of the size of our downstream GCC patchset. I don't understand how this relates to fixing a known bug upstream. We are actively trying to get musl compatibility issues fixed upstream to reduce the size of our downstream patchset, which is why I pinged this again.

On a side note, I would also like to point out that most of our patches are quite small. This particular patches changes ~400 lines across 17 files and would thus be by far our biggest patch if we end up backporting it eventually (ideally after the outstanding comments have addressed and this patch has been merged upstream).

ibuclaw reviewed Feb 27, 2021

View reviewed changes

n8sh reviewed Mar 2, 2021

View reviewed changes

src/core/stdc/time.d Show resolved Hide resolved

omerfirmak force-pushed the dmd-cxx-time64 branch from c29b303 to 7066b97 Compare March 2, 2021 06:25

omerfirmak force-pushed the dmd-cxx-time64 branch from 7066b97 to 1161108 Compare March 2, 2021 06:34

RazvanN7 mentioned this pull request Mar 2, 2021

CRuntime_Musl: More fixes for time64 #3313

Closed

kinke mentioned this pull request Mar 2, 2021

CRuntime_Musl: More fixes for time64 #3384

Open

ibuclaw suggested changes Mar 12, 2021

View reviewed changes

dlang-bot added the Needs Rebase needs a `git rebase` performed label May 27, 2021

ibuclaw self-assigned this Jun 6, 2021

dlang-bot added the stalled label Jun 10, 2021

dlang-bot removed the stalled label Apr 28, 2022

CyberShadow mentioned this pull request Jan 9, 2023

Fix linking to 64-bit time_t versions of functions on musl dlang/dmd#14794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRuntime_Musl: fixes for time64 #3383

CRuntime_Musl: fixes for time64 #3383

omerfirmak commented Feb 26, 2021

dlang-bot commented Feb 26, 2021

ibuclaw Feb 27, 2021

ibuclaw Mar 2, 2021

ibuclaw Feb 27, 2021

ibuclaw Feb 27, 2021

Geod24 Mar 2, 2021

RazvanN7 commented Mar 3, 2021

RazvanN7 commented Mar 12, 2021

ibuclaw left a comment

Geod24 commented Mar 12, 2021

RazvanN7 commented Apr 28, 2022

nmeum commented Aug 12, 2022 •

edited

thewilsonator commented Aug 12, 2022

thewilsonator commented Aug 12, 2022

ibuclaw commented Aug 13, 2022 •

edited

nmeum commented Aug 13, 2022

CRuntime_Musl: fixes for time64 #3383

Are you sure you want to change the base?

CRuntime_Musl: fixes for time64 #3383

Conversation

omerfirmak commented Feb 26, 2021

dlang-bot commented Feb 26, 2021

Bugzilla references

Testing this PR locally

ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

ibuclaw Mar 2, 2021

Choose a reason for hiding this comment

ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

Geod24 Mar 2, 2021

Choose a reason for hiding this comment

RazvanN7 commented Mar 3, 2021

RazvanN7 commented Mar 12, 2021

ibuclaw left a comment

Choose a reason for hiding this comment

Geod24 commented Mar 12, 2021

RazvanN7 commented Apr 28, 2022

nmeum commented Aug 12, 2022 • edited

thewilsonator commented Aug 12, 2022

thewilsonator commented Aug 12, 2022

ibuclaw commented Aug 13, 2022 • edited

nmeum commented Aug 13, 2022

nmeum commented Aug 12, 2022 •

edited

ibuclaw commented Aug 13, 2022 •

edited