Show failing instruction on SIGILL #22134
Conversation
|
@jlbuild !filter=arm,ppc,aarch64 |
src/signal-handling.c
Outdated
| { | ||
| #if defined(_CPU_X86_64_) || defined(_CPU_X86_) | ||
| uintptr_t pc = 0; | ||
| # if defined(_OS_LINUX_) && defined(_CPU_X86_64_) |
There was a problem hiding this comment.
The indentation in the preprocessor logic here is weird and seems to be inconsistent with other files (e.g. debuginfo.c), which don't use indentation on nested conditions.
There was a problem hiding this comment.
There are quite a few of these in the codebase. I usually use them when the nested one has more than one condition/blocks. Distinguish them with comments gets harder in these cases.
src/signal-handling.c
Outdated
| } | ||
| } | ||
| #else | ||
| # if defined(_OS_FREEBSD_) || defined(_OS_DARWIN_) |
There was a problem hiding this comment.
I always knew you had a soft spot for FreeBSD 😉
There was a problem hiding this comment.
FWIW, this one was due to macOS CI....
src/signal-handling.c
Outdated
| # else | ||
| unsigned char mvec; | ||
| # endif | ||
| valid = mincore((void*)next_page, jl_page_size, &mvec) != -1; |
There was a problem hiding this comment.
I've found that if (msync((void*)(fault_addr & ~(jl_page_size - 1)), 1, MS_ASYNC) != 0) is more accurate (tests whether the kernel has defined a memory region for that virtual address, rather than testing if that address currently resides in physical memory)
src/signal-handling.c
Outdated
| int len = sizeof(inst); | ||
| // since we got a SIGILL and not a SIGSEGV or SIGBUS assume that | ||
| // the `pc` is pointed to valid memory. | ||
| // However, this does not mean that `pc + 14` is valid memory. |
There was a problem hiding this comment.
this implementation seems sort of crazy to have all in one functions. maybe we should have a safe_read function? (mapping to, on macOS mach_vm_read_overwrite; on windows and linux read(mmap); and on others, jl_safe_try { memcpy })
There was a problem hiding this comment.
Maybe I'll just do a loop of volatile byte read with safe context setup?
There was a problem hiding this comment.
yeah, that's what I meant by my last option
There was a problem hiding this comment.
Yes, I agree this could be factored into a few functions. Also get_pc_from_ctx.
|
I just go with the catch segfault approach. It's simplest to implement, useful in other cases and I don't actually expect any segfault here in >99% of the cases anyway.... |
|
@jlbuild !filter=arm,ppc,aarch64 |
yuyichao
force-pushed
the
yyc/signals/ill
branch
2 times, most recently
from
54df26f
to
f575d63
Compare
|
@jlbuild !filter=arm,ppc,aarch64 |
No description provided.