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

druntime: redirect dual-ABI functions on glibc to IEEE128 version #20826

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Feb 6, 2025

This pull request redirects powerpc dual-ABI functions on glibc to the IEEE128 version if IEEE long double ABI is selected.

A new (reserved) version identifier, D_PPCUseIEEE128, is introduced to control the redirection behaviour. Compilers are expected to set this version identifier when the user selects IEEE 128 ABI for their project.

Required compiler changes (setting the D_PPCUseIEEE128 identifier) are also proposed in the following:

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @liushuyu! 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#20826"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

please use aliases instead of pragma(mangle, ...), like:

pragma(scanf)
     int __isoc99_scanf(scope const char* format, scope ...);
     ///
     alias scanf = __isoc99_scanf;
``

@liushuyu
Copy link
Contributor Author

liushuyu commented Feb 6, 2025

please use aliases instead of pragma(mangle, ...), like:

pragma(scanf)
     int __isoc99_scanf(scope const char* format, scope ...);
     ///
     alias scanf = __isoc99_scanf;
``

I don't know if I understand you correctly, but

extern (C) void a();

version (PPC64) {
    static if (real.mant_dig == 113) {
        extern (C) void a__ieee128();
        alias a = a__ieee128;
    }
}

void b() {
    a();
}

This does not work. b will still call a instead of a__ieee128 even if the conditions are met.
LDC and GDC also do not issue any warnings or errors in this case.

@liushuyu
Copy link
Contributor Author

liushuyu commented Feb 6, 2025

Assuming we can't do anything about the alias problem in the compiler, an alternative may be doing this (also taking the suggestions from the LDC pull request):

version (PPC64)
    enum PPCUseIEEE128 = real.mant_dig == 113;
else
    enum PPCUseIEEE128 = false;

static if (PPCUseIEEE128)
{
    real __strtoieee128(scope inout(char)* nptr, scope inout(char)** endptr);
    alias strtold = __strtoieee128;
}
else
{
    real strtold(scope inout(char)* nptr, scope inout(char)** endptr);
}

... which is a bit verbose for math.d as there are a lot of functions to be aliased.

@thewilsonator
Copy link
Contributor

You only need to have one static if and one else branch, you can put multiple declarations for the different function is there.

@liushuyu
Copy link
Contributor Author

liushuyu commented Feb 7, 2025

You only need to have one static if and one else branch, you can put multiple declarations for the different function is there.

I know, but my concern is that declaring each function twice may increase maintenance burden (both versions have the same D function signature), as opposed to using mangle only needs one function definition

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

Successfully merging this pull request may close these issues.

3 participants