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 behavior around jumps with immediate values > 0x7fffffff in the interpreter #447

Merged
merged 5 commits into from May 8, 2024

Conversation

Alan-Jowett
Copy link
Collaborator

Resolves: #444

Interpreter wasn't correctly sign extending the immediate value before comparing it to the register.

Alan Jowett added 2 commits May 7, 2024 10:11
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
@Alan-Jowett Alan-Jowett requested a review from hawkinsw May 7, 2024 18:23
@Alan-Jowett Alan-Jowett enabled auto-merge (squash) May 7, 2024 18:23
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
@coveralls
Copy link

coveralls commented May 7, 2024

Coverage Status

coverage: 81.241% (+0.006%) from 81.235%
when pulling 8b003aa on Alan-Jowett:unsigned_jump_test
into db3e2d3 on iovisor:main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be a separate commit?

* @return The sign extended 64-bit value.
*/
static int64_t sign_extend_immediate(int32_t immediate) {
return (int64_t)immediate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am currently working through whether or not this is guaranteed to give the proper behavior according to the C standard. In newer versions of the C standard, twos complement representation of signed integers is specified. In older versions, it is not. Therefore, we will (I think) need to be very careful about this. See, e.g., https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2218.htm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Further research:

  1. GCC represents integers as twos complement: https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/Integers-implementation.html#Integers-implementation
  2. MSVC represents integers as twos complement: https://learn.microsoft.com/en-us/cpp/c-language/range-of-integer-values?view=msvc-170

There does not appear to be a place where Clang (llvm) defines its implementation-specific behavior. See llvm/llvm-project#11644 for the status of such a document.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the yet-to-be standardized version of C, twos complement is specified as the representation to use for signed integers. See https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf

vm/ubpf_vm.c Outdated Show resolved Hide resolved
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
Copy link
Collaborator

@hawkinsw hawkinsw left a comment

Choose a reason for hiding this comment

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

I think that I am happy that what we have written will sign extend properly in the case of the three most common compilers on the platforms that we support.

* @return The sign extended 64-bit value.
*/
static int64_t sign_extend_immediate(int32_t immediate) {
return (int64_t)immediate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the yet-to-be standardized version of C, twos complement is specified as the representation to use for signed integers. See https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf

@Alan-Jowett Alan-Jowett disabled auto-merge May 8, 2024 03:35
@Alan-Jowett Alan-Jowett merged commit e8de891 into iovisor:main May 8, 2024
34 checks passed
@Alan-Jowett Alan-Jowett deleted the unsigned_jump_test branch May 8, 2024 03:35
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.

uBPF interpreter has incorrect behavior for jump with immediate values > 0x7fffffff
3 participants