-
Notifications
You must be signed in to change notification settings - Fork 143
Use new assembly syntax #165
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
Conversation
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 a lot for the pull request! I have some minor comments, but overall this looks good to me.
Absolutely! This would be very useful for changes like this.
Done! |
All of those have been fixed, feel free to merge when you want |
Sorry for the late reply! The code looks good to me overall, but the
I'm not sure what's the cause of this, but it seems like something goes wrong with the IDT or TSS loading. |
I believe that's caused by the code that reloads |
@rybot666 Any updates on this? |
Sorry I totally forgot about this! I'll take another look |
No worries, thanks! |
For some reason |
There was a breaking change in the Rust repository lately (change of the directory layout), so you might need to update your nightly version and/or
If you're on a newer nightly, you need |
src/instructions/segmentation.rs
Outdated
llvm_asm!("pushq $0; \ | ||
leaq 1f(%rip), %rax; \ | ||
pushq %rax; \ | ||
lretq; \ | ||
1:" :: "ri" (u64::from(sel.0)) : "rax" "memory"); | ||
// Returns | ||
unsafe fn ret_sym() { | ||
asm!("ret"); | ||
} | ||
|
||
// Pushes the new CS and the address of the "ret" function | ||
asm!("push {cs}; lea {out_reg}, [{ret_sym}]; push {out_reg}; retf", | ||
cs = in(reg) u64::from(sel.0), ret_sym = sym ret_sym, out_reg = out(reg) _); |
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 think this might be why we were seeing test failures. We can just directly use the existing implementation with the new asm!
syntax:
asm!(
"push {cs}",
"lea {tmp}, [1f + rip]",
"push {tmp}",
"retfq",
"1:",
cs = in(reg) u64::from(sel),
tmp = lateout(reg) _,
options(nomem, preserves_flags),
);
I compiled this locally, and got the same assembly output as the existing implementation.
e8981b0
to
5dc5f74
Compare
@phil-opp @rybot666 I rebased this, implemented the missing commands, and fixed the PTAL |
5dc5f74
to
c3c0c35
Compare
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.
The updates look reasonable to me, but I didn't look at the set_cs
assembly in detail. The fact that the tests pass again is a good sign though.
I disassembled the double_fault_stack_overflow test, and the code that sets movzx ecx,WORD PTR [rax+0x48]
push rcx
lea rax,[rip+0x3]
push rax
rex.W retf
mov rdi,rbx So this PR (at least) didn't mess things up. |
Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
c3c0c35
to
f171cea
Compare
Great, thanks a lot for checking! |
This pull request updates
x86_64
to use the newasm!
macro. This will break older nightlies, but makes the syntax easier to read.Fixes #164