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

config/arm: use eabihf for hard float #4023

Merged
merged 2 commits into from
Jan 8, 2020
Merged

config/arm: use eabihf for hard float #4023

merged 2 commits into from
Jan 8, 2020

Conversation

MilhouseVH
Copy link
Contributor

Following a discussion on zlib-ng/zlib-ng#478 it became apparent that the TARGET_ABI used for arm doesn't reflect the presence of hard float.

I have successfully created images for RPi, RPi2 and Allwinner/A64 with this PR (RPi and RPi2 images also booted OK). Add-ons are yet to be build tested.

Creating as a draft for discussion, and in case any additions are required.

@@ -13,7 +13,7 @@
generic|cortex-a35|cortex-a53|cortex-a57|cortex-a72|exynos-m1|qdf24xx|thunderx|xgene1|cortex-a57.cortex-a53|cortex-a72.cortex-a53)
TARGET_SUBARCH=aarch64
TARGET_VARIANT=armv8-a
TARGET_ABI=eabi
TARGET_ABI=eabihf
Copy link
Contributor

Choose a reason for hiding this comment

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

huh? there is no soft float abi on aarch64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sooo... are you agreeing with this change? Regardless of whether there is a soft float abi or not, the suggestion is that we should be including the hf suffix in the hard float abi.

Copy link
Contributor

Choose a reason for hiding this comment

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

The aarch64 change is probably unnecessary. zlib-ng's hardfloat test appears to be looking for eabihf only for arm. There's a check further down which assumes floating point is true on aarch64. No floating point support (march=armv8-a+nofp(+nosimd?)) is possible, but I haven't seen it in the wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks required to me - this is the zlib-ng code in question:

https://github.com/zlib-ng/zlib-ng/blob/3ea747396dcc3d63924eedbdf05bacf7866e1b29/CMakeLists.txt#L202-L214

where BASEARCH_ARM_FOUND is:

https://github.com/zlib-ng/zlib-ng/blob/3ea747396dcc3d63924eedbdf05bacf7866e1b29/cmake/archdetect.cmake#L51-L53

Therefore setting hard or soft float by matching eabihf occurs for BOTH arm AND aarch64.

I can test this later today and confirm what happens (I may have already done this, but forgotten!) - I can build Allwinner/A64 (which is using aarch64) with eabi (ie. without this PR) and I'm fairly certain zlib-ng will try to use soft float. Allwinner/A64 definitely builds successfully with this PR and zlib-ng.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way I'm not saying what zlib-ng is doing is entirely correct or sensible (it's certainly wrong in places with bias towards armv8 etc.), but if hard float should be reflected in the os triplet then that's something we're not currently doing and can add easily enough hopefully without any issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

arm change is ok. aarch64 is not and should not be added. it makes no sense to add just because random library's cmake buildsystem is (semi-)broken or makes wrong assumptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we've been discussing this on LE Slack and have come to the same conclusion, I'll remove the aarch64 change. BTW I've posted our/my concerns to zlib-ng, here - would appreciate your thoughts if want to post there! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aarch64 change now dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, they could just trust distro's compiler / CFLAGS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I suggested. 😄

@dhewg
Copy link
Contributor

dhewg commented Dec 3, 2019

I've been testing this for a few days now for a couple of builds on Allwinner/H6. Looks good to me, no regressions here

@MilhouseVH MilhouseVH marked this pull request as ready for review December 14, 2019 02:45
@chewitt chewitt merged commit d3323bb into LibreELEC:master Jan 8, 2020
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.

None yet

5 participants