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

Generalize and extend unit tests working with binary files #2372

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kevemueller
Copy link

  • Fix shlib_list_from_elf_hints to work with FreeBSD files on non-FreeBSD systems.
  • Introduce unit test helper function to describe golden values of bundled test binaries.
  • Simplify abi unit test to use helper
  • Introduce create-parsebin unit test that packages binaries and validates their manifest.

@kevemueller
Copy link
Author

This is a draft PR to start the discussion on shlibs handling.

The added tests/frontend/create-parsebin:create_from_binbase test case fails on non-native systems as pkg_elf.filter_system_shlibs insists on finding the NEEDED libraries.

IMO if a library is referenced, it should go into shlibs_required, whether it exists, or not. So the test case is right, the current behaviour is wrong.

Happy to discuss here.

@kevemueller
Copy link
Author

The test passes on FreeBSD by chance only. E.g. on a FreeBSD14 Aarch64 binary it would "match" the FreeBSD14 amd64 library provided by the runner and go on.
It would similarly fail when packaging an old FreeBSD binary on a recent FreeBSD runner, when library versions mismatch.

@ifreund
Copy link
Contributor

ifreund commented Dec 1, 2024

IMO if a library is referenced, it should go into shlibs_required, whether it exists, or not. So the test case is right, the current behaviour is wrong.

I agree with this. I think the only reason to filter shlibs is to respect the ALLOW_BASE_SHLIBS option not being set to true. This is far from the only issue with the current shlib handling code though. I have a concrete plan for the direction to take here and a WIP implementation but haven't gotten it review/merge ready yet.

While working on that I encountered many issues with pkg's internal handling of ABI information, which I have nearly finished cleaning up. (sneak peek here, need to fix a few regressions and clean up the comments)

When that's done (hopefully tomorrow), I plan to return to my implementation of #2331 which will be made significantly easier to implement through my internal ABI handling improvements. Having tests for shlib tracking sounds amazing!

@kevemueller
Copy link
Author

Well, I sees the clarifications in ABI processing. Would have benefited from them :)
Please don't leave out amd64-x32, and aarch64-x32, these are valid FreeBSD MACHINE_ARCH names, that are currently not used.

If we ever were	to support the so-called x32 ABI (using 32-bit	pointers on the	 amd64 architecture),
it  would  most	 likely	 be encoded as amd64-x32.

Just FYI I am now making the change to remove pkg_macho.c, so we will have some merges to be done.

The code to create shlib regression tests is already in this commit and it is "early" stage.
Our test binaries currently don't use shared libraries, so I could only work with libc...

Keve added 3 commits December 1, 2024 22:27
* Fix shlib_list_from_elf_hints to work with FreeBSD files on non-FreeBSD systems.
* Introduce unit test helper function to describe golden values of bundled test binaries.
* Simplify abi unit test to use helper
* Introduce create-parsebin unit test that packages binaries and validates their manifest.
* Add analyse_macho to read and populate shared libraries
* Remove legacy pkg_macho code.
@kevemueller
Copy link
Author

I have now added to this draft the Mach-O shlibs code as well as the removal of the legacy Mach-o code.
This supersedes/replaces #2367.

Please advise if you want to disable the failing ELF tests or keep them failing so the code is being picked up.

I have temporarily added the shlibs enabled MacOS test binaries built using the Makefile from #2373.

@ifreund
Copy link
Contributor

ifreund commented Dec 2, 2024

Please don't leave out amd64-x32, and aarch64-x32, these are valid FreeBSD MACHINE_ARCH names, that are currently not used.

Sure, will do. I don't think I really understand what these architectures are though. Don't all amd64 processors have 32 bit compatibility support? What's the difference between amd64 and amd64-x32?

Just FYI I am now making the change to remove pkg_macho.c, so we will have some merges to be done.

No worries, I don't mind resolving conflicts if you get your code finished first :)

@kevemueller
Copy link
Author

X32 or more generally ILP32 is using the advanced instruction sets available on the modern (64bit) architecture, but only 32bit adresses. E.g. https://wiki.debian.org/X32Port

Will unlikely happen in FreeBSD, but it is there. More common under RISCV and PPC, but in general people have accustomed themselves to 64bit address spaces.

The Mach-O code is in the PR.

@kevemueller kevemueller marked this pull request as ready for review December 2, 2024 11:52
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.

2 participants