Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

CRuntime_Musl: fixes for time64 #3383

Open
wants to merge 1 commit into
base: dmd-cxx
Choose a base branch
from

Conversation

omerfirmak
Copy link
Contributor

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
Copy link
Contributor

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 Outdated Show resolved Hide resolved
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, when running the testsuite.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
enum SO_TIMESTAMP = 63;
enum SO_TIMESTAMPNS = 64;
enum SO_TIMESTAMPING = 65;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 <[email protected]>
@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 3, 2021

@Geod24 What is your stance on this?

@RazvanN7
Copy link
Contributor

@ibuclaw Is it ok to merge this?

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Geod24 commented Mar 12, 2021

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

@dlang-bot dlang-bot added the Needs Rebase needs a `git rebase` performed label May 27, 2021
@ibuclaw ibuclaw self-assigned this Jun 6, 2021
@RazvanN7
Copy link
Contributor

@Geod24 any progress on this?

@dlang-bot dlang-bot removed the stalled label Apr 28, 2022
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Aug 11, 2022
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
Copy link

nmeum commented Aug 12, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

ibuclaw commented Aug 13, 2022

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
Copy link

nmeum commented Aug 13, 2022

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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants