-
Notifications
You must be signed in to change notification settings - Fork 130
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
ubpf_vm: Add mem_len via R2 into uBPF VM #58
Conversation
I see that #22 also required a chance to the JIT code. However my testing suggests this change is not needed? Also, how do I add a testcase for this change? |
The tests currently pass without it, but if you add a new test to the |
And for adding a test, just look in |
fcd5fe8
to
d2527b5
Compare
@jpsamaroo I did what you suggest. I cherry-picked @ygrek and added a test. Note that this test is really simple. However while it passes for the VM it fails for the x86_64 JIT and I am not sure why. Can you take a look? |
Oh @jpsamaroo I think we have a problem here. After reviewing the JIT code some more it looks like RSI is used to return a divide by zero error handle. So we might have to review that. https://github.com/iovisor/ubpf/blob/master/vm/ubpf_jit_x86_64.c#L446 |
I don't see any evidence that the div-by-zero handler is the issue. The div-by-zero handler only gets hit if you have a div (otherwise we just do the regular epilogue), which you don't have in your test. Maybe try throwing that test under gdb and see why this is happening? |
The issue is with the register mapping. When we only shift by one (ie -r 1) then map_register[1] is RSI and thus RSI gets overwritten by RDI before RSI is copied to map_register[2]. This could be fixed with some complicated logic. But my recommendation would be to just drop the transfer and add a "-- no register offset" to any test which relies on the second argument. (Like is done for call instructions). The register offset test is of limited use here anyway and register offsets are only useful for the test code. |
d2527b5
to
ffde802
Compare
@lsgunth thanks! @jpsamaroo I updated the mem-length.data test to disable the register offset testing. It should now pass. Can you take a look and LMK if this looks good to merge? |
vm/ubpf_jit_x86_64.c
Outdated
/* Move rsi into register 2 */ | ||
if (map_register(2) != RSI) { | ||
emit_mov(state, RSI, map_register(2)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we can't rely on the length register when applying a register offset, I think these lines should just be removed.... Keeping them here just confusingly makes it seem like that would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpsamaroo can you comment on this. It looks like @lsgunth is correct here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that we'll already be passing the memory length in RSI anyway thanks to calling conventions, so there's no need for this 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jpsamaroo. I will drop these lines and resubmit.
It is useful for programs to be able to operate based on the length ofthe input data passed in to it. Pass in this length via the R2 register. [cherry-pick of b7caed9 with some modifications to commit message] Fixes iovisor#57.
Add a simple test to ensure the mem_len variable is being passed into the VM and/or JIT as expected. Note that due to how the register offset testing works we need to disable it for this new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@pchaigno I think this looks good to merge. |
Thanks for this patch :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This is going to be a highly needed change in my fork, so let's get it merged.
It is useful for programs to be able to operate based on the length of
the input data passed in to it. Pass in this length via the R2
register.
Fixes #57.