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

Add dedicated zero-extension and sign-extension instructions #529

Closed
wants to merge 1 commit into from

Conversation

elazarg
Copy link
Collaborator

@elazarg elazarg commented Sep 22, 2023

Fix #63, which should also help with issues described in #479.

Pairs of rX = rX << 32; rX = rX >> 32 are translated to a single zero-extension instruction (if the second instruction is RSH) or sign-extension instruction (if the second instruction is ARSH).
The translation is done during unmarshalling, similarly to LDDW instructions.
Semantically, zext is NOP if the unsigned value fits in 32 bits, and sext is nop if the signed value fits in 32 bits.
Otherwise, fall back to handling of left+right shift (which does not maintain relational invariants).

It may be possible to be more precise, but I don't think we need it at this point.

Signed-off-by: Elazar Gershuni <[email protected]>
@elazarg elazarg requested a review from caballa September 22, 2023 12:09
@coveralls
Copy link

Coverage Status

coverage: 88.676% (+0.02%) from 88.654% when pulling 8fe3744 on zext-sext into 3300cea on main.

@dthaler
Copy link
Contributor

dthaler commented Sep 25, 2023

Issue #523 is relevant where there are now BPF instructions for sign extension

@@ -60,6 +60,8 @@ static uint8_t imm(Un::Op op) {
case Op::LE32: return 32;
case Op::BE64:
case Op::LE64: return 64;
case Op::ZEXT32:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think ZEXT needs a new instruction, the existing BPF_MOV | BPF_ALU (0xbc) should already do zero-extension.

Comment on lines +71 to +72
{"zext32", Un::Op::ZEXT32},
{"sext32", Un::Op::SEXT32},
Copy link
Contributor

Choose a reason for hiding this comment

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

there are multiple new signed extension instructions that are Bin ops (not Un ops), so I don't think there's any reason to add Un ops here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think sext32 can now be done using the MOVSX instruction

@dthaler
Copy link
Contributor

dthaler commented Feb 12, 2024

FYI, I just started working on an update to this PR that will reuse now-existing instructions. It may take me until next week as I'm out of town for half the week.

@elazarg elazarg closed this Feb 14, 2024
Alan-Jowett pushed a commit to Alan-Jowett/ebpf-verifier that referenced this pull request Oct 15, 2024
When register R12 is used in an instruction requiring a ModRM byte, the
semantics and encoding of the instruction are unique. The
emit_modrm_and_displacement function did not handle this uniqueness
properly. This patch modifies the function to handle the use of R12
correctly.

Fixes vbpf#529

Signed-off-by: Will Hawkins <[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.

Merge x <<= 32; x >>= 32 to x = sext x
3 participants