Page MenuHomePhabricator

[Mips] Always save RA when disabling frame pointer elimination
ClosedPublic

Authored by jrtc27 on Oct 5 2019, 12:55 PM.

Details

Summary

This ensures that frame-based unwinding will continue to work when
calling a noreturn function; there is not much use having the caller's
frame pointer saved if you don't also have the caller's program counter.

Diff Detail

Repository
rL LLVM

Event Timeline

jrtc27 created this revision.Oct 5 2019, 12:55 PM
jrtc27 updated this revision to Diff 223398.Oct 5 2019, 5:28 PM

Updated existing tests

jrtc27 updated this revision to Diff 223399.Oct 5 2019, 5:29 PM

Stray whitespace

Harbormaster completed remote builds in B39050: Diff 223399.
draganm added a subscriber: draganm.Oct 6 2019, 4:35 AM
jrtc27 added a comment.Oct 6 2019, 9:09 AM

Note that the AArch64, ARM, PowerPC (32/64/64le), RISCV (32/64) and SystemZ backends all do this (Sparc gets it by virtue of register windowing so it half counts, and similarly X86 due to the hardware stack push), at least with my C test case (which was reduced to give the IR test case addd here); Mips is currently the odd one out.

atanasyan accepted this revision.Oct 7 2019, 6:57 AM

LGTM. Thanks for the patch.

This revision is now accepted and ready to land.Oct 7 2019, 6:57 AM

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

Agreed. But saving RA has a low cost, might be useful and as far as I can see - gcc saves RA in the same cases.

jrtc27 added a comment.Oct 7 2019, 8:26 AM

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

I see. I haven't loked further but just removing nounwind from callee makes caller save $ra. Thanks for the concrete example. The patch makes sense to me now.

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

Would probably be useful to include this example in the commit message.

This revision was automatically updated to reflect the committed changes.