Stop backtracing if the stack pointer gets stuck #135804
Conversation
rustbot
added
S-waiting-on-review
|
Actually this wouldn't really work with something like stacker as the new stack segment may have a lower address than the old one. Maybe this check could be only done if the jump is less than say 1MB? Also there are architectures where the stack grows up rather than down. For those the check needs to be reversed. |
|
Hm, the implementation here doesn't really care which way the stack pointer moves so long as it does move. I.e. it just checks that the stack pointer is not equal the previous one. |
|
Right, in that case it should be fine. |
| // If we're using a short backtrace, ignore all frames until we're told to start printing. | ||
| let mut print = print_fmt != PrintFmt::Short; | ||
| set_image_base(); | ||
| // SAFETY: we roll our own locking in this town | ||
| unsafe { | ||
| backtrace_rs::trace_unsynchronized(|frame| { | ||
| // Break if the stack pointer does not move (see #135717). | ||
| // Make sure to skip the first frame to handle the case where the frame pointer is omitted. | ||
| if frame.sp() == last_sp && !frame.sp().is_null() && idx > 1 { |
There was a problem hiding this comment.
How do "inline frames" get reported? Eg, if fn a calls fn b and b is inlined into a, then I could see the stack pointer for the "inline frame" b having the same address as frame a's stack pointer if a panic were to happen inside b.
There was a problem hiding this comment.
This is only for the trace itself so inline frames shouldn't be handled until symbolization, no? (i.e. resolve_frame_unsynchronized below this code)
There was a problem hiding this comment.
I could be totally wrong, but I seem to recall that dbghelp.dll would sometimes report inlined frames as if they were actual frames (perhaps only on i686 or something like that?)
There was a problem hiding this comment.
Oh hm the old StackWalk64 API could be a problem, yes. I'll investigate.
There was a problem hiding this comment.
Looking at our backtrace code, it seems that we do expect to always handle inline frames in symbolization. There's even a fallback for StackWalk64's lack of InlineFrameContext https://github.com/rust-lang/backtrace-rs/blob/016f80ae2179fdd8479db179cf47ed16a1198422/src/symbolize/dbghelp.rs#L160). I would assume that would have very weird results were inline frames to be reported as actual frames. Though I've not yet been able to find anything conclusive.
There was a problem hiding this comment.
I think I implemented part of that last year in rust-lang/backtrace-rs#569. Maybe I'm just remembering something I ran into during development of that patch. If your test case works ok on i686 with inlining happening, then I don't have any concerns with this change 🙂
There was a problem hiding this comment.
In my testing I've not been able to provoke any problems but I'll see what the full CI says. It is entirely possible there's a situation I'm not accounting for.
There was a problem hiding this comment.
Ok, so I just realised I was testing either with full debug info or no debug info but not with line-tables-only. This does show an issue on i686. I'll investigate further.
There was a problem hiding this comment.
Ah I need to track both AddrStack and InlineFrameContext and only stop if both are the same. Which will need me to come up with a backtrace API for this.
There was a problem hiding this comment.
Yeah, that sounds correct to me!
ChrisDenton
force-pushed
the
backtrace-wine
branch
from
e838792 to
f39c4c8
Compare
|
Given potential concerns, I'll mark this as not to be rolled up. @bors r=joboet rollup=never |
bors
added
S-waiting-on-bors
|
@bors r- I think I may have found an issue, |
bors
added
S-waiting-on-author
If the stack pointer does not make progress when backtracing then something has gone wrong and we should just stop rather than potentially continuing forever.
Workaround for #135717