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

ci: arm64 support #11

Merged
merged 6 commits into from
Mar 12, 2025
Merged

ci: arm64 support #11

merged 6 commits into from
Mar 12, 2025

Conversation

theihor
Copy link
Contributor

@theihor theihor commented Feb 14, 2025

No description provided.

@anakryiko
Copy link
Member

@theihor do we still need this PR?

@theihor
Copy link
Contributor Author

theihor commented Mar 5, 2025

@theihor do we still need this PR?

@anakryiko if we are ok with using "unofficial" arm64 build of bpftrace, I can update this PR to add arm64 to CI. Let me know.

@anakryiko
Copy link
Member

@theihor do we still need this PR?

@anakryiko if we are ok with using "unofficial" arm64 build of bpftrace, I can update this PR to add arm64 to CI. Let me know.

please remind me what are the implications? will it break often?

@theihor
Copy link
Contributor Author

theihor commented Mar 5, 2025

please remind me what are the implications? will it break often?

@anakryiko the arm64 bpftrace binary is not an officially supported release. So it is probably less tested.

For this CI, getting it is not a simple wget from an authoritative source. Whenever we'll want to update it, it's a little bit more work than a version bump. One option (used in this PR for testing) is to create a fake release (like this one) whenever we want to update bpftrace.

If it's not that important for bpftrace to be up to date, then I'd suggest to stick to the version that I tried. I don't expect much problems with it, and if I'm wrong about that we'll notice quickly.

@anakryiko
Copy link
Member

Yeah, freshness of bpftrace isn't that important, we don't use anything bleeding-edge or fancy here. Let's do the custom release and have arm64 support in CI, thanks!

@theihor theihor force-pushed the ci-arm64 branch 2 times, most recently from 1aba64d to e0bea81 Compare March 6, 2025 18:07
@theihor theihor changed the title Test bpftrace arm64 binary ci: arm64 support Mar 6, 2025
@theihor theihor force-pushed the ci-arm64 branch 5 times, most recently from b2601d3 to d708a36 Compare March 6, 2025 18:54
@theihor theihor marked this pull request as ready for review March 6, 2025 20:42
@theihor
Copy link
Contributor Author

theihor commented Mar 6, 2025

@anakryiko I had to fix a few things in awk magic, and disable struct-by-value test for arm too. See the commits for details.

@theihor theihor requested a review from anakryiko March 6, 2025 20:44
theihor added 5 commits March 11, 2025 10:48

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
Rewrite install-bpftrace.sh script to download custom bpftrace-arm64
release [1] and use it for testing on aarch64.

Add github-hosted arm64 runners to the test workflow matrix.

[1] https://github.com/theihor/bpftrace/releases/tag/v0.22.1-arm64

Signed-off-by: Ihor Solodrai <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
On arm64, argument spec of a tracepoint does not include dollar
signs. In tests matching expected arguments exactly, add USDT specs
for arm64.

Signed-off-by: Ihor Solodrai <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
Modify the condition for arg_types struct-by-value test case to only
run on x86_64 with gcc.

Signed-off-by: Ihor Solodrai <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
Signed-off-by: Ihor Solodrai <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
NF in awk relies on whitespace to split the "fields" in a line.
This lead to incorrectly counted argn in the following case:

    Arguments: -4@[sp, 40]

Change how argn is computed: count /-?[0-9]+@/ matches in a
line (examples: '-4@[sp, 40]', '8@%rdi'),

Signed-off-by: Ihor Solodrai <[email protected]>
Copy link
Member

@anakryiko anakryiko left a comment

Choose a reason for hiding this comment

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

LGTM, left a tiny nit, but otherwise, let's land it! Thanks for working on this!

Comment on lines 16 to 19
if (base != 0) {
base_key = filename ":" base;
if (!(base_key in basemap)) {
basemap[base_key] = ++base_cnt;
Copy link
Member

Choose a reason for hiding this comment

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

let's reduce nestedness:

base_key = filename ":" base;
if (base != 0 && !(base_key in basemap)) {
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Verified

This commit was signed with the committer’s verified signature.
theihor Ihor Solodrai
The base and semaphore offsets between different binaries may
coincide, which leads to incorrect matching of USDTs by test scripts.

Use filename + offset string to identify them for the purpose of USDT
spec comparison.

readelf -n prints the file path before dumping notes if more than one
file is passed in, and this is now captured by fetch-usdts.awk.

Signed-off-by: Ihor Solodrai <[email protected]>
@anakryiko anakryiko merged commit 769965f into libbpf:main Mar 12, 2025
28 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.

None yet

2 participants