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

Fix build on FreeBSD/powerpc* #299

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

Conversation

kgotlinux
Copy link

All the tests pass.

@Mizux
Copy link
Collaborator

Mizux commented Feb 22, 2023

FYI x86 implem do support Linux(or Android) and FreeBSD but sources are splitted...

So do we keep the same file "layout" for PowerPC ? or rename the file impl_ppc_linux_or_freebsd.c
note: I don't remember why we split the sources
EDIT:

// Handling FreeBSD platform through parsing /var/run/dmesg.boot.
const int fd = CpuFeatures_OpenFile("/var/run/dmesg.boot");

// Handling Linux platform through /proc/cpuinfo.
const int fd = CpuFeatures_OpenFile("/proc/cpuinfo");

@gchatelet
Copy link
Collaborator

For FreeBSD we need to parse /var/run/dmesg.boot instead of /proc/cpuinfo and the format is not the same.
So it's definitely not the same way of detecting the features.
Tests "pass" because we create a fake filesystem with /proc/cpuinfo, but it won't exist on a real FreeBSD so this patch is incorrect.

@pkubaj
Copy link

pkubaj commented Feb 23, 2023

While the current implementation with /proc/cpuinfo is wrong, parsing /var/run/dmesg.boot is also wrong even if it works. On FreeBSD you need to use elf_aux_info() to get CPU capabilities. It works similarly to Linux's getauxval().

@gchatelet
Copy link
Collaborator

While the current implementation with /proc/cpuinfo is wrong, parsing /var/run/dmesg.boot is also wrong even if it works. On FreeBSD you need to use elf_aux_info() to get CPU capabilities. It works similarly to Linuxs getauxval()`.

Ah! Thx a lot for the information. I was unaware of elf_aux_info().
We'll look into this. @pkubaj do you know if the function is always available or if it needs dynamic loading as for android?

@pkubaj
Copy link

pkubaj commented Feb 23, 2023

This function is available without dynamic loading. You just need FreeBSD 12.0 or newer and include sys/auxv.h. An example of using it would be https://github.com/zlib-ng/zlib-ng/pull/1340/files

@kgotlinux
Copy link
Author

Pull request rebased and fixed, It currently builds on the support added in 89a3f03, which adds support for using elf_aux_info() on FreeBSD. Currently list_cpu_features correctly lists architecture and CPU features, It does not list CPU model, microarchitecture etc., due to no AT_PLATFORM on FreeBSD.

@toor1245
Copy link
Contributor

toor1245 commented Sep 23, 2023

@kgotlinux, could you please add src/impl_ppc_freebsd.c file to BUILD.bazel: https://github.com/google/cpu_features/blob/main/BUILD.bazel#L237
https://github.com/google/cpu_features/blob/main/BUILD.bazel#L302

and make sure the following command will work fine?

bazel run list_cpu_features 

@gchatelet gchatelet added bug Something isn't working compilation error Code does not compile labels Sep 25, 2023
@gchatelet gchatelet added this to the v0.10.0 milestone Sep 25, 2023
@gchatelet
Copy link
Collaborator

gchatelet commented Sep 25, 2023

@kgotlinux can you clang format the c file?

EDIT: never mind, I'll do it.

kgotlinux and others added 2 commits September 25, 2023 08:44
This builds on the code added in 89a3f03. Since AT_PLATFORM is not available on FreeBSD,
currently only architecture detection and CPU feature detection work.
@toor1245
Copy link
Contributor

@gchatelet, I see bug and compilation error labels, but we didn't have support FreeBSD powerpc at all, so I would say this is new architecture support for FreeBSD.

@gchatelet gchatelet added enhancement New feature or request and removed bug Something isn't working compilation error Code does not compile labels Sep 25, 2023
@gchatelet
Copy link
Collaborator

@gchatelet, I see bug and compilation error labels, but we didn't have support FreeBSD powerpc at all, so I would say this is new architecture support for FreeBSD.

Fair enough, thx for noticing.
Let's try to add CI support for this patch before merging.

@Mizux would you have a bit of time to add CI for FreeBSD / Power ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants