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

Follow-up for masks on shift operations #105

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Follow-up for masks on shift operations #105

merged 2 commits into from
Mar 15, 2024

Conversation

qmonnet
Copy link
Owner

@qmonnet qmonnet commented Mar 15, 2024

  • tests/ubpf_vm.rs: Add tests for arsh offset masking
  • src/interpreter: Remove/explain masks for 32-bit shift operations

Cc: @pcy190

Add tests to make sure we handle the masking of the value (in the source
register or as an immediate) telling by how many bits the value in the
destination register should be shifted.

The context is that we used to do no masking on this value, but the BPF
ISA specification does require one, and the kernel implements it when
the value is passed in the source register (and rejects immediates that
it deems invalid).

Signed-off-by: Quentin Monnet <qmo@qmon.net>
We recently fixed the 64-bit arithmetic shift-right operations by adding
a mask to the number of bits (as immediates or from the source register)
by which to shift, as per the eBPF ISA specification. The 32-bit
operations must use the same mask, but .wrapping_shr() already takes
care of it for us. Let's add a comment to make it explicit.

As it turns out, masking is just as well unnecessary for the
non-arithmetic left-right shift operations that we tried to fix
recently. Let's also remove the mask there.

Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet qmonnet merged commit 3e80ed7 into main Mar 15, 2024
7 checks passed
@qmonnet qmonnet deleted the arsh-fix-tests branch March 15, 2024 12:11
@pcy190
Copy link
Contributor

pcy190 commented Mar 15, 2024

Many thanks for the follow-up!

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