Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

CyberShadow
Copy link
Member

@CyberShadow CyberShadow commented Jan 8, 2023

musl switched to 64-bit time_t across all architectures in version 1.2.0:

https://musl.libc.org/time64.html

This change was done in a way which attempted to preserve ABI compatibility. To achieve this, the 32-bit versions of functions were left at their original names in the compiled library, and new 64-bit versions of functions were introduced. The header files then redirected calls to the standard function names to use the new 64-bit versions using the __asm__("name") construct, which is similar to D's pragma(mangle, "name").

This change is a fix-up for commit ca0b670, which tried addressing this change in musl by changing time_t to 64-bit when targeting new musl versions (the default). However, that change was incomplete, as it did not implement the function redirection part of the change, which is required to actually call the implementations using 64-bit time_t. As a result, it caused programs to link but return incorrect results at runtime on 32-bit architectures when targeting new musl versions.

Fix this by adjusting the mangled name of the D declarations of affected functions when targeting musl on 32-bit platforms. Affected functions in musl can be found by grepping for _REDIR_TIME64 and uses of the __REDIR macro.


In dlang/druntime#3275, @Geod24 wrote:

but we needed to fix this to avoid random memory corruption

I'm guessing what was fixed by that change is not interop between D and the musl libc, but between D and other C libraries which use the definition of time_t from the libc. dlang/druntime#3275 causes Druntime to divide by zero at startup - because we are calling clock_getres with the expectation that it populates a timespec with 64-bit time_t, but we are actually calling the 32-bit version, so ts.tv_nsec remains at the default-initialized 0 value.

CC @Geod24 @Cogitri @ibuclaw

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 8, 2023

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close Bugzilla Severity Description
23608 regression [musl 32-bit] Time functions linked incorrectly on musl >=1.2.0 / 32-bit

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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#14794"

@CyberShadow CyberShadow force-pushed the pull-20230108-182916 branch 2 times, most recently from 57a8308 to 62d942d Compare January 8, 2023 19:36
@CyberShadow CyberShadow marked this pull request as ready for review January 8, 2023 19:37
@CyberShadow
Copy link
Member Author

CC @omerfirmak, I see you tried fixing this as well in dlang/druntime#3383

musl switched to 64-bit time_t across all architectures in version
1.2.0:

https://musl.libc.org/time64.html

This change was done in a way which attempted to preserve ABI
compatibility. To achieve this, the 32-bit versions of functions were
left at their original names in the compiled library, and new 64-bit
versions of functions were introduced. The header files then
redirected calls to the standard function names to use the new 64-bit
versions using the __asm__("name") construct, which is similar to D's
pragma(mangle, "name").

This change is a fix-up for commit
ca0b670, which tried addressing this
change in musl by changing time_t to 64-bit when targeting new musl
versions (the default). However, that change was incomplete, as it did
not implement the function redirection part of the change, which is
required to actually call the implementations using 64-bit time_t. As
a result, it caused programs to link but return incorrect results at
runtime on 32-bit architectures when targeting new musl versions.

Fix this by adjusting the mangled name of the D declarations of
affected functions when targeting musl on 32-bit platforms.  Affected
functions in musl can be found by grepping for _REDIR_TIME64 and uses
of the __REDIR macro.

Fixes issue 23608.
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

I'm a bit worried about future-proofness, but thank you for fixing this!

@Geod24
Copy link
Member

Geod24 commented Feb 19, 2023

Stable though ?

@CyberShadow
Copy link
Member Author

In theory, future C APIs introduced by musl will all use 64-bit time_t exclusively, so we won't need mangleof hacks for them.

I can rebase on stable. This seems like a potentially high impact change and the regression was a while ago so targeting master seems safer?

@Geod24
Copy link
Member

Geod24 commented Feb 19, 2023

I think it'll be better to get this rolled out in the packages sooner than later. We currently have most architectures disabled on Alpine and that's a step in the right direction.

@CyberShadow
Copy link
Member Author

Ah, great! I will open a new PR to avoid the mass ping.

@CyberShadow
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants