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

sys-devel/binutils-apple: fix build on macOS 13, including ARM64. #30781

Closed
wants to merge 1 commit into from

Conversation

biergaizi
Copy link
Contributor

This commits introduces three patches to fix build on macOS 13 (Intel), and also on ARM64 (Apple Silicon).

  1. On macOS 13, Gentoo Prefix fails to compile cctools due to missing Blocks support in GCC, creating the following error:
/Users/ec2-user/gentoo/MacOSX.sdk/usr/include/objc/runtime.h:373:29: error: expected ')' before '^' token
  373 |                       void (^ _Nonnull block)(Class _Nonnull aClass, BOOL * _Nonnull stop)
      |                             ^
      |                             )

Fortunately the header objc-runtime.h is not strictly necessary. As a workaround, stop including objc-runtime.h in print_objc.c and print_bitcode.c.

  1. In ar(1), strcmp() checks are used to determine the value of argument argv[1], even when no argument is given. In the past, they were possibly harmless out-of-bound reads and comparison with garbage, without consequences.

However, running it on macOS 13 w/ Apple Silicon immediately crashes it with Segmentation Fault, because argv[1] is now NULL and generates EXC_BAD_ACCESS in strcmp().

$ ./bin/ar
Segmentation fault: 11

This commit checks whether argc is equal or greater than 2 before doing strcmp(). No revbump is needed since the package couldn't be built on ARM64 before.

  1. Fabian Groffen's downstream patch "fix compilation on arm64", which adds missing CPU register definitions.

Closes: 905131

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @biergaizi
Areas affected: ebuilds
Packages affected: sys-devel/binutils-apple

sys-devel/binutils-apple: @gentoo/prefix

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Apr 27, 2023
@biergaizi biergaizi force-pushed the macos13-binutils-apple branch 2 times, most recently from fc8be0e to cbf3f84 Compare April 27, 2023 20:51
This commits introduces three patches to fix build on macOS 13
(Intel), and also on ARM64 (Apple Silicon).

1. On macOS 13, Gentoo Prefix fails to compile cctools due to
missing Blocks support in GCC, creating the following error:

/Users/ec2-user/gentoo/MacOSX.sdk/usr/include/objc/runtime.h:373:29: error: expected ')' before '^' token
  373 |                       void (^ _Nonnull block)(Class _Nonnull aClass, BOOL * _Nonnull stop)
      |                             ^
      |                             )

Fortunately the header objc-runtime.h is not strictly necessary.
As a workaround, stop including objc-runtime.h in print_objc.c
and print_bitcode.c.

2. In ar(1), strcmp() checks are used to determine the value
of argument argv[1], even when no argument is given. In the
past, they were possibly harmless out-of-bound reads and
comparison with garbage, without consequences.

However, running it on macOS 13 w/ Apple Silicon immediately
crashes it with Segmentation Fault, because argv[1] is now
NULL and generates EXC_BAD_ACCESS in strcmp().

$ ./bin/ar
Segmentation fault: 11

This commit checks whether argc is equal or greater than 2
before doing strcmp(). No revbump is needed since the package
couldn't be built on ARM64 before.

3. Fabian Groffen's downstream patch "fix compilation on arm64",
which adds missing CPU register definitions.

Closes: https://bugs.gentoo.org/905131
Signed-off-by: Yifeng Li <tomli@tomli.me>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-04-27 20:54 UTC
Newest commit scanned: 47127b9
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/0d0fa74e56/output.html

run_ranlib = 1;

if (argc < 3) {
- if(strcmp(argv[1], "--version") == 0){
Copy link
Member

Choose a reason for hiding this comment

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

This changes what it compiles to, so please revbump for this even if it didn't crash before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@thesamesam thesamesam requested a review from grobian April 27, 2023 20:58
@biergaizi
Copy link
Contributor Author

How should this package be revbumped properly? Its "upstream" is in fact a downstream repo that Fabian Groffen maintained specially for use in Gentoo, so its "upstream" version number is already "gentoo-8.2.1-r101" that contains "r101". Should I merge these patches into Groffen's repo instead?

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-04-27 21:04 UTC
Newest commit scanned: f17f786
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/53f076c9bd/output.html

@biergaizi
Copy link
Contributor Author

biergaizi commented Apr 27, 2023

Which revision number should I use for this ebuild? r102? r101-r1? I'm afraid that r102 has a risk of clashing with a future git tag in Fabian Groffen's repo, which is currently r101.

@thesamesam
Copy link
Member

thesamesam commented Apr 28, 2023

I'm going to let grobian answer because I don't want to complicate things if he has a different idea in mind.

Personally, in this case, I'd say we should swap the "upstream" (fork/grobian repo) versioning to _pN, and use -rN for Gentoo revisions.

@grobian
Copy link
Member

grobian commented Apr 28, 2023

hmmm, I once gave it a spin and then got to the conclusion that we were missing a lot of arm64 stuff, but you're saying it actually works with these reasonably small changes?

I noticed Ian has a newer version that I meant to look at to see if we can upgrade to it.

In any case, I maintain indeed a fork for our usage, so let's patch it there, and then spin a new release.

@biergaizi
Copy link
Contributor Author

biergaizi commented Apr 28, 2023

hmmm, I once gave it a spin and then got to the conclusion that we were missing a lot of arm64 stuff, but you're saying it actually works with these reasonably small changes?

I'm mot sure about the functionality of every single tool, possibly there are still problems, but at least it can be built from source now, Portage can even link programs with it, so I'd call it progress.

@biergaizi
Copy link
Contributor Author

biergaizi commented Apr 28, 2023

I just opened another Pull Request to submit the ar(1) patch. After that change, binutils-apple, let's make a new release.

@grobian
Copy link
Member

grobian commented May 29, 2023

I've pushed these fixes as binutils-apple-8.2.1-r102

@grobian grobian closed this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits.
Projects
None yet
5 participants