Integrate deoptimisation into guard failures. #460
Conversation
c_tests/run.rs
Outdated
| @@ -103,6 +103,8 @@ fn mk_compiler(exe: &Path, src: &Path, opt: &str, extra_objs: &[PathBuf]) -> Com | |||
| "-Wall", | |||
| // Enable LTO with lld. | |||
| "-fuse-ld=lld", | |||
| // Don't remove frame pointers as we'll need them for guard failures. | |||
| "-fno-omit-frame-pointer", | |||
There was a problem hiding this comment.
So we couldn't address from the stack pointer? I guess it complicates things a lot?
There was a problem hiding this comment.
If I remember correctly this is needed so we can get the return address with __builtin_return_address(0).
There was a problem hiding this comment.
Before we turn frame pointers off (which, AIUI, can have a big effect on optimisations), we should see if there's another way of doing this.
There was a problem hiding this comment.
Looks like there's a RA register on x64 which supposedly contains the return address. Let me see if that works.
There was a problem hiding this comment.
That's the first I've heard of this. Got a link?
There was a problem hiding this comment.
Ah, so fno-omit-frame-pointer stores the return address in the RA register. With fomit-frame-pointer this register can be used as general purpose register, which explains why this may have a performance implication. So we may have to read the return address from the stack manually.
|
Think that's all comments addressed. Should we squash and then wait for @ltratt to have a look? |
|
@ltratt is bound to have queries and comments. I think we should let him have at least a quick look. |
|
Yes, I just thought it's less distracting for him if we squash away the already addressed stuff first. |
| addr: *const usize, | ||
| } | ||
|
|
||
| impl Registers { |
There was a problem hiding this comment.
I think the functions in here, especially as they're unsafe, need doc strings (even if short ones).
ykcapi/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn get(&self, id: u16) -> usize { |
There was a problem hiding this comment.
Presumably this function (and maybe others?) should be #[cfg(target_arch = "x86_64")].
ykcapi/src/lib.rs
Outdated
| for l in locs { | ||
| match l { | ||
| SMLocation::Register(reg, size) => { | ||
| eprintln!("Register: {} ({} {})", registers.get(*reg), reg, size); |
There was a problem hiding this comment.
Can we add FIXMEs before all of these printlns so that they're more likely to be removed sooner rather than later. As you know, I am allergic to debug prints, which are like zombies: they don't die unless extra effort is put into killing them!
ykcapi/yk.h
Outdated
| @@ -48,4 +48,43 @@ YkLocation yk_location_new(void); | |||
| // will occur. | |||
| void yk_location_drop(YkLocation); | |||
|
|
|||
| void yk_stopgap(void *addr, uintptr_t size, uintptr_t retaddr, void *rsp); | |||
|
|
|||
| void __llvm_deoptimize(void* addr, uintptr_t size) { | |||
There was a problem hiding this comment.
Should this be #[cfg(target_arch = "x86_64")]?
ykllvmwrap/src/ykllvmwrap.cc
Outdated
| // Allocate space for compiled trace address, stackmap address, and stackmap | ||
| // size (plus padding). | ||
| // FIXME This is a temporary hack until the redesigned hot location is up. | ||
| uintptr_t *ptr = (uintptr_t *) malloc(32); |
There was a problem hiding this comment.
Let's at least make this malloc(sizeof(uintptr_t) * 3).
There was a problem hiding this comment.
I assume that the other 8 bytes are the padding the comment mentions?
There was a problem hiding this comment.
What about the padding?
There was a problem hiding this comment.
Oh, seems like padding isn't needed for malloc.
Ah, let's reference that PDF then! Also it's probably worth clearing up why we have both |
The doc string for |
|
With @vext01's help found a way to keep Ready for re-review I think. |
|
I have no comments beyond @vext01's comments. |
|
As and when @vext01 is happy, please squash. |
|
Just 3 more commits for you to review @ltratt and we are ready. |
|
Please squash. |
|
Okay to squash? |
|
Ready for re-review. |
|
LGTM. |
|
You need to resolve merge conflicts. |
|
Squashed and rebased. |
|
bors r= |
|
bors r+ |
|
Build failed: |
|
Huh, this is odd. The tests succeed if I run them manually. Investigating. |
|
The failing tests are release tests. Could it be that we don't set the debug printing flag in release mode? |
|
This should fix it. |
|
Please squash. |
|
Squashed! |
|
bors r+ |
|
Build failed: |
|
Of course I only checked the rust formatting, not the C formatting. Sorry. Ok to squash? |
|
Please squash. |
Until now we've simply crashed when we encountered a guard failure. This commit integrates the previously added stackmap parser into our guard failures so we can read out the live variables at that point. This is another step towards proper stopgapping, though until we have a stopgap interpreter we only print out the live variables we've found.
|
Squashed. |
|
bors r+ |
|
Build succeeded: |
Until now we've simply crashed when we encountered a guard failure. This
PR integrates the previously added stackmap parser into our guard
failures so we can read out the live variables at that point. This is
another step towards proper stopgapping, though until we have a stopgap
interpreter we only print out the live variables we've found.