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

[BPF] fix print_btf.py test script on bigendian machines #110332

Merged
merged 1 commit into from
Sep 29, 2024

Conversation

eddyz87
Copy link
Contributor

@eddyz87 eddyz87 commented Sep 27, 2024

Make print_btf.py correctly detect endianness of the BTF input. Input endianness is inferred from BTF magic word [2], which is a 2-byte integer at offset 0 of the input:

  • sequence EB 9F signals big-endian input;
  • sequence 9F EB signals little-endian input.

Before this commit the magic sequence was read using "H" format for unpack method of python's struct module:

  • if magic is 0xEB9F assume little-endian;
  • if magic is 0x9FEB assume big-endian.

However, format H reads data in native endianness.

Thus the above logic would only be correct on little endian hosts:

  • byte sequence 9F EB read as 0xEB9F -> little-endian input;
  • byte sequence EB 9F read as 0x9FEB -> big-endian input.

On the big-endian host the relation should be inverse.

Fix this by always reading magic in big-endian (format >H).

This fixes CI error reported for a BPF test using print_btf.py script in [1]. The error happens on a s390 host, which is big-endian.

[1] https://lab.llvm.org/buildbot/#/builders/42/builds/1192
[2] https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-type-and-string-encoding

@eddyz87
Copy link
Contributor Author

eddyz87 commented Sep 27, 2024

@uweigand, could you please take a look? As far as I understand this has to land asap and I know that Yonghong is PTO today, and I need someone to review the change.

@eddyz87
Copy link
Contributor Author

eddyz87 commented Sep 27, 2024

(for the record, the change was tested locally on a big-endian virtual machine, as described here)

Copy link
Member

@uweigand uweigand left a comment

Choose a reason for hiding this comment

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

The explanation makes sense to me, and the patch LGTM. Thanks!

@eddyz87
Copy link
Contributor Author

eddyz87 commented Sep 27, 2024

The explanation makes sense to me, and the patch LGTM. Thanks!

Thank you, will land once CI passes.

@yonghong-song
Copy link
Contributor

Thanks @eddyz87 fixing the issue. I don't think the test failure is due to this patch. Maybe rebase on top of main branch and try again.

@eddyz87
Copy link
Contributor Author

eddyz87 commented Sep 28, 2024

Thanks @eddyz87 fixing the issue. I don't think the test failure is due to this patch. Maybe rebase on top of main branch and try again.

Rebased and pushed, let's see what happens.

@eddyz87
Copy link
Contributor Author

eddyz87 commented Sep 29, 2024

Looks like CI issues are caused by #107638, it would be reverted soon: #110384.

Make print_btf.py correctly detect endianness of the BTF input.
Input endianness is inferred from BTF magic word [2], which is a
2-byte integer at offset 0 of the input:
- sequence `EB 9F` signals big-endian input;
- sequence `9F EB` signals little-endian input.

Before this commit the magic sequence was read using "H" format for
`unpack` method of python's `struct` module:
- if magic is `0xEB9F` assume little-endian;
- if magic is `0x9FEB` assume big-endian.

However, format `H` reads data in native endianness.

Thus the above logic would only be correct on little endian hosts:
- byte sequence `9F EB` read as `0xEB9F` -> little-endian input;
- byte sequence `EB 9F` read as `0x9FEB` -> big-endian input.

On the big-endian host the relation should be inverse.

Fix this by always reading magic in big-endian (format `>H`).

This fixes CI error reported for a BPF test using print_btf.py script
in [1]. The error happens on a s390 host, which is big-endian.

[1] https://lab.llvm.org/buildbot/#/builders/42/builds/1192
[2] https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-type-and-string-encoding
@eddyz87 eddyz87 merged commit 26df43f into llvm:main Sep 29, 2024
8 checks passed
@uweigand
Copy link
Member

The SystemZ build bots are all green again now, thanks!

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
Make print_btf.py correctly detect endianness of the BTF input. Input
endianness is inferred from BTF magic word [2], which is a 2-byte
integer at offset 0 of the input:
- sequence `EB 9F` signals big-endian input;
- sequence `9F EB` signals little-endian input.

Before this commit the magic sequence was read using "H" format for
`unpack` method of python's `struct` module:
- if magic is `0xEB9F` assume little-endian;
- if magic is `0x9FEB` assume big-endian.

However, format `H` reads data in native endianness.

Thus the above logic would only be correct on little endian hosts:
- byte sequence `9F EB` read as `0xEB9F` -> little-endian input;
- byte sequence `EB 9F` read as `0x9FEB` -> big-endian input.

On the big-endian host the relation should be inverse.

Fix this by always reading magic in big-endian (format `>H`).

This fixes CI error reported for a BPF test using print_btf.py script in
[1]. The error happens on a s390 host, which is big-endian.

[1] https://lab.llvm.org/buildbot/#/builders/42/builds/1192
[2]
https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-type-and-string-encoding
ingomueller-net added a commit to ingomueller-net/llvm-project that referenced this pull request Oct 14, 2024
This PR revisits the traits and interfaces of the `alternatives`,
`foreach`, and `yield`ops. First, it adds the `ReturnLike` trait to
`transform.yield`. This is required in the one-shot bufferization pass
since the merging of llvm#110332, which analyses any `FunctionOpInterface`
and expects them to have a `ReturnLike` terminator.

This uncovered problems with `alternatives` and `foreach`, which
implemented the `BranchRegionOpInterface`. That interface verifies that
the operands and results passed across the control flow edges from the
parent op to its regions and back are equal (or compatible).
Which results are passed back from regions to the parent op depend on
the terminator and/or whether it is `ReturnLike`. Making `yield` a
`ReturnLike` op, those checks fail without other changes.

We explored and rejected the possibility to fix the implementation of
`RegionBranchOpInterface` of the two concerned ops in llvm#111408. The
problem is that the interface expects the result passed from a region to
the parent op to *be* the result of the op, whereas in the two ops they
*contribute* to the result: in `alternatives` only the operand yielded
from one of the regions becomes the result whereas the others are
ignored, and in `foreach` the operands from all iterations are fused
into a new value that becomes the result of the op.

Signed-off-by: Ingo Müller <[email protected]>
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.

3 participants