stdlib: define typealias CLongDouble for FreeBSD #61756
Conversation
|
Are you able to run the compiler validation suite on FreeBSD with this pull? What results do you get? |
Not yet, since there are other issues keeping me from even building the compiler + stdlib. I'm basically working through them as I hit them and trying to provide targeted patches. #61661 (merged) However, once I get things building, I do plan to move on to running the test suite to find any further bugs. If there's a more appropriate way to contribute these fixes, I'm happy to change my process. Please let me know—thanks! |
|
Because of the difficulty of getting in small patches like this for a non-committer like you, I suggest you get the compiler validation suite running on FreeBSD first then roll all these changes into one pull, similar to how @3405691582 did for libdispatch, swiftlang/swift-corelibs-libdispatch#559, and report your test results with it. |
|
See also #59988, which may or may not be helpful to you. |
| @@ -101,6 +101,8 @@ public typealias CLongDouble = Double | |||
| #endif | |||
| #elseif os(OpenBSD) | |||
| public typealias CLongDouble = Float80 | |||
| #elseif os(FreeBSD) | |||
| public typealias CLongDouble = Float80 | |||
There was a problem hiding this comment.
This seems wrong. FP80 is X86 only according to https://www.freebsd.org/cgi/man.cgi?query=arch&sektion=7&format=html. This should be similar to what we have for Darwin.
There was a problem hiding this comment.
Good point, thanks!
I think that's also true for OpenBSD:
$ for os in freebsd openbsd; do
for arch in i386 x86_64 armv7 arm64; do
target="${arch}-${os}"
echo -n "${target}: "
clang-15 -target "${target}" -E -dM - </dev/null | fgrep __LDBL_MANT_DIG__
done
echo
done
i386-freebsd: #define __LDBL_MANT_DIG__ 64
x86_64-freebsd: #define __LDBL_MANT_DIG__ 64
armv7-freebsd: #define __LDBL_MANT_DIG__ 53
arm64-freebsd: #define __LDBL_MANT_DIG__ 113
i386-openbsd: #define __LDBL_MANT_DIG__ 64
x86_64-openbsd: #define __LDBL_MANT_DIG__ 64
armv7-openbsd: #define __LDBL_MANT_DIG__ 53
arm64-openbsd: #define __LDBL_MANT_DIG__ 113
I've added an x86_64 || i386 conditional to the FreeBSD branch. @compnerd & @3405691582, would you like me to do the same for OpenBSD, or would you prefer I leave it alone (or just add a comment)?
There was a problem hiding this comment.
A separate PR for fixing O/BSD would be appreciated!
There was a problem hiding this comment.
Currently it is an error to attempt to build Swift on anything but amd64-openbsd so it's not strictly necessary to fix for this platform right now. It might be worthwhile doing, but it might be misleading if you see the conditional and yet get a cmake error trying to bring up the platform on the architecture though.
mhjacobson
force-pushed
the
freebsd-define-CLongDouble-typealias
branch
from
1a97bd6 to
f873569
Compare
| @@ -101,6 +101,11 @@ public typealias CLongDouble = Double | |||
| #endif | |||
| #elseif os(OpenBSD) | |||
| public typealias CLongDouble = Float80 | |||
| #elseif os(FreeBSD) | |||
| #if arch(x86_64) || arch(i386) | |||
There was a problem hiding this comment.
My preference would be to swap the architectures here, but that is low priority.
There was a problem hiding this comment.
Happy to—I was just matching the Darwin and Linux branches.
stdlib/public/core/CTypes.swift
Outdated
| #if arch(x86_64) || arch(i386) | ||
| public typealias CLongDouble = Float80 | ||
| #endif | ||
| // TODO: Fill in definitions for additional architectures as needed. |
There was a problem hiding this comment.
This should alias to Double here. I think we might as well as do that now rather than wait for that.
There was a problem hiding this comment.
For arm64-freebsd, at least, long double is float128, which would be different from Double. This is similar to what's going on in the Linux branch above:
// TODO: Fill in definitions for additional architectures as needed. IIRC
// armv7 should map to Double, but arm64 and ppc64le should map to Float128,
// which we don't yet have in Swift.
I figured I'd match that in spirit, no?
There was a problem hiding this comment.
(As a general review point, It also means there is a sensible default or #error rather than causing an error implicitly for a missing type alias. The suggestion to use Double suffices, though.)
There was a problem hiding this comment.
In this case it would be even worse, it would be a weird error when trying to import a C function that uses a long double in its signature. So, what @3405691582 points out is pretty critical. I think that just handling the case properly with the alias to Double (which is FP64 and thus matches the ABI) is the proper thing to do.
There was a problem hiding this comment.
Agreed about the weird error—that's precisely what I hit and prompted this PR. 🙂
I've updated this PR to #error in cases where CLongDouble isn't defined. Let me know what you think.
I think that just handling the case properly with the alias to Double (which is FP64 and thus matches the ABI) is the proper thing to do.
Double doesn't always match, though. For example, freebsd-arm64 uses float128 for long double (see my __LDBL_MANT_DIG__ experiment in a different comment).
mhjacobson
force-pushed
the
freebsd-define-CLongDouble-typealias
branch
from
f873569 to
4800fa0
Compare
stdlib/public/core/CTypes.swift
Outdated
| #endif | ||
| #elseif os(Android) | ||
| // On Android, long double is Float128 for AAPCS64, which we don't have yet in | ||
| // Swift (https://github.com/apple/swift/issues/51573); and Double for ARMv7. | ||
| #if arch(arm) | ||
| public typealias CLongDouble = Double | ||
| #else | ||
| #error("CLongDouble needs to be defined for this Android architecture") |
There was a problem hiding this comment.
As noted above, it is float128 on AArch64, which otherwise works and this is going to break compilation on, same with linux aarch64 above.
There was a problem hiding this comment.
I am fine with the BSD errors below, but please don't touch linux and Android, as those are already working for aarch64 despite not supporting float128, or require this to be defined for all platforms.
There was a problem hiding this comment.
OK, I'm happy with that if everyone else is. Pushed that change now.
Any opinions about the other-OS branch (which I guess would encompass PS4, Cygwin, Haiku, and WASI)?
There was a problem hiding this comment.
As I said, "don't... require this to be defined for all platforms," as some platforms like linux aarch64 don't define it and work fine, ie remove the last OS error.
mhjacobson
force-pushed
the
freebsd-define-CLongDouble-typealias
branch
from
4800fa0 to
fc3b5e8
Compare
This fixes a failure compiling the stdlib where the importer cannot find (i.e., refuses to import) functions _stdlib_remainderl and _stdlib_squareRootl.
mhjacobson
force-pushed
the
freebsd-define-CLongDouble-typealias
branch
from
fc3b5e8 to
802d877
Compare
|
@swift-ci please smoke test |
This fixes a failure compiling the stdlib where the importer cannot find (i.e., refuses to import) functions _stdlib_remainderl and _stdlib_squareRootl.