Page MenuHomePhabricator

[DebugInfo][InstrRef] Cope with win32 calls changing the stack-pointer in LiveDebugValues

Authored by jmorse on Nov 23 2021, 7:58 AM.



This patch works around an edge case in win32 environments that could produce false addresses for variable length arrays.

Most of LiveDebugValues (VarLoc and InstrRef) is written with the assumption that register defs of the stack pointer on call instructions should be ignored -- the call instruction certainly alters SP, but the stack is balanced when the call returns [0]. Unfortunately there's a single scenario where this definitely isn't true: on 32-bit Windows, with variable-length arrays, we call a function _chkstk that both probes the stack and alters the stack pointer. With DBG_VALUE based variable locations, this isn't a huge problem: a DBG_VALUE is applied to the stack pointer after the function returns, and is then copied to other registers. However in instruction referencing mode, we need to instrument the instruction defining the value (see next patch): that means the call to _chkstk, which violates the above assumption that calls don't define the stack pointer.

This patch adds an exception: we add a target frame-lowering hook to see whether stack probe functions will modify the stack pointer, store that in an internal flag, and if it's true then scan CALL instructions to see whether they're such badly-behaved functions. If they are, we recognise them as defining a new stack-pointer value. If we don't do this, then LiveDebugValues will always think "the dynamic allocas address is always whatever the current stack pointer is", which is going to produce false locations if more than one dynamic alloca is used.

The added test exercises this behaviour: two calls to _chkstk should be considered as producing two different values.

[0] This isn't true of stdcall, but I think we have existing difficulties with that anyway.

Diff Detail

Event Timeline

jmorse created this revision.Nov 23 2021, 7:58 AM
jmorse requested review of this revision.Nov 23 2021, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 7:58 AM
Orlando accepted this revision.Nov 24 2021, 7:37 AM

I'm definitely not a windows expert by any stretch, so wouldn't object to a second opinion on this and the follow-up patch. But everything you've said makes sense and the changes LGTM (with a few inline nits).

[0] This isn't true of stdcall, but I think we have existing difficulties with that anyway.



Any reason not to stuff this all inside the IgnoreSPAlias lambda? Not a strong opinion.


nit: does- > do


nit: target's

This revision is now accepted and ready to land.Nov 24 2021, 7:37 AM
jmorse added inline comments.Nov 24 2021, 9:32 AM

Performance considerations -- the lambda might be called a lot, wouldn't want to use strcmp repeatedly when it can instead be used once.

This revision was landed with ongoing or failed builds.Nov 24 2021, 11:56 AM
This revision was automatically updated to reflect the committed changes.