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

script/lib.sh: set GOARM=5 for armel, GOARM=6 for armhf #4034

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

AkihiroSuda
Copy link
Member

https://github.com/golang/go/wiki/GoArm#supported-operating-systems

ARM on Linux. You must run an EABI kernel.
These are generally known as armel for softfloat (compatible with ARMv5)
or armhf for hardware floating point (ARMv6 and above).

Prior to this commit, the script was setting GOARM=6 for armel, GOARM=7 for armhf.

Fix #4033

@AkihiroSuda
Copy link
Member Author

https://wiki.debian.org/ArmHardFloatPort

In practice armel will be used for older CPUs (armv4t, armv5, armv6), and armhf for newer CPUs (armv7+VFP).

Debian wiki conflicts with Golang wiki.
Not sure which one is correct.

@AkihiroSuda
Copy link
Member Author

https://wiki.debian.org/RaspberryPi

Raspberry Pi OS builds a single image for all of the Raspberry families, so you will get an armhf 32-bit, hard floating-point system, but built for the ARMv6 ISA (with VFP2), unlike Debian's ARMv7 ISA (with VFP3) port.

Looks like armhf means ARMv7 for Debian, ARMv6 for Raspbian 🤯 .

So it looks like this PR still makes a sense for Raspbian users.

"armhf" means ARMv7 for Debian, ARMv6 for Raspbian.
ARMv6 is chosen here for compatibility.

https://wiki.debian.org/RaspberryPi

> Raspberry Pi OS builds a single image for all of the Raspberry families,
> so you will get an armhf 32-bit, hard floating-point system, but built
> for the ARMv6 ISA (with VFP2), unlike Debian's ARMv7 ISA (with VFP3)
> port.

Prior to this commit, the script was setting GOARM=6 for armel,
GOARM=7 for armhf.

Fix issue 4033

Signed-off-by: Akihiro Suda <[email protected]>
@kolyshkin
Copy link
Contributor

Originally, these values for GOARM (6 and 7) come from PR #1820 (commit 39f679c).

My guess is https://wiki.debian.org/ArmHardFloatPort (last modified in 2017) is wrong/outdated to say armhf requires v7, since the other two sources say v6.

OTOH https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html says

Note that floating-point is not supported by the base ARMv7-M architecture, but is compatible with both the ARMv7-A and ARMv7-R architectures.

Meaning, even v7 can require soft-float, depending on the hardware. So, it's a maze.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Also, since no one ever complained about how we build our binaries, maybe we are doing it right?

@tianon
Copy link
Member

tianon commented Sep 27, 2023

The Debian wiki isn't outdated; rather within Debian (read: Raspbian is not Debian), there is officially no v6 (hence the Raspbian rebuild), only v5 (armel) and v7 (armhf). It might be better to use less ambiguous names like armv5, armv6, armv7, but generally armv5 binaries will work fine on both v5 and v6 devices, so this change is sane IMO.

@tianon
Copy link
Member

tianon commented Sep 27, 2023

For (up to date) baselines within Debian, https://wiki.debian.org/ArchitectureSpecificsMemo#Architecture_baselines is probably a better reference.

@kolyshkin
Copy link
Contributor

It might be better to use less ambiguous names like armv5, armv6, armv7

Yes, I think this is what we should do @AkihiroSuda, instead of using armel and armhf we should use armv5, armv6, armv7 (and choose hf for the last two).

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe we should change the naming as suggested by @tianon

@AkihiroSuda
Copy link
Member Author

LGTM, but maybe we should change the naming as suggested by @tianon

Not in this PR, as it will break third-party installation scripts that curls runc

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda requested a review from cyphar September 27, 2023 23:01
@kolyshkin kolyshkin merged commit ee45b9b into opencontainers:main Sep 28, 2023
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

armel should use GOARM=5, not GOARM=6
4 participants