[Lldb-commits] [PATCH] D26295: Change UnwindAssemblyInstEmulation to remove a register location instead of marking it as IsSame()

[Tamas Berghammer via lldb-commits]

I think the UnwindAssemblyInstEmulation tracks every register without
knowing if it is volatile or not and then RegisterContextLLDB will consult
with the ABI (during RegisterRead) to decide which registers can it recover
(based on volatile/non-volatile). For x86 I think the long term solution
would be to migrate it over to UnwindAssemblyInstEmulation instead of the
custom code what should solve the issue you mentioned.

Regarding IsSame I think it would be a useful thing if we can somehow
differentiate between a volatile register can be overwritten by something
(e.g. a call) or we know that it can be recovered using a specific rule.
Once I hit an issue on aarch64 that a hand written assembly function inside
the "[vdso]" used a volatile register in its (hand written) eh_frame as the
author know that it isn't clobbered but LLDB failed to unwind as it said
the register is volatile. My view is that making the dumps easier to read
is a very small gain (only help people working on the unwinder) so I don't
want to change the logic just for that but if IsSame and being unspecified
are equivalent then getting rid of IsSame would be a nice code
simplification.

On Fri, Nov 4, 2016 at 10:41 PM Jason Molenda <jmolenda at apple.com> wrote:

> jasonmolenda added a comment.
>
> Ah, interesting point, I didn't think of that.  However, this touches on
> another thing I've been thinking about as I look at the assembly inspection
> unwind plan generators.  In the x86 unwind inspector, I've hardcoded the
> SysV-x86_64 ABI and the unwind plan generator ignores any saves/restores of
> volatile registers.  It's a poor choice and it's the kind of thing that
> surely won't be correct when a Windows port is up & running.
>
> I'm thinking the unwind plan generators should treat all registers as
> non-volatile.  When UnwindLLDB / RegisterContextLLDB run the UnwindPlan,
> they can ask the ABI if a register is volatile or not - and refuse to
> retrieve a volatile register for a stack frame in the middle of the stack.
> (it SHOULD be doing that already)
>
> The problem with tracking a register that is volatile is that as soon as
> the function makes a call into another function, we have to assume the
> register value is overwritten.  So if we have
>
>   0xfffffff021f7bd80 <+0>:    stp    x28, x27, [sp, #-0x60]!
>   0xfffffff021f7bd84 <+4>:    stp    x26, x25, [sp, #0x10]
>   0xfffffff021f7bd88 <+8>:    stp    x24, x23, [sp, #0x20]
>   0xfffffff021f7bd8c <+12>:   stp    x22, x21, [sp, #0x30]
>   0xfffffff021f7bd90 <+16>:   stp    x20, x19, [sp, #0x40]
>   0xfffffff021f7bd94 <+20>:   stp    x29, x30, [sp, #0x50]
>   0xfffffff021f7bd98 <+24>:   add    x29, sp, #0x50            ; =0x50
>   0xfffffff021f7bd9c <+28>:   sub    sp, sp, #0xe0             ; =0xe0
>   0xfffffff021f7bdd4 <+84>:   bl     0xfffffff021f8af70
>
>   0xfffffff021f7c334 <+1460>: str    w9, [sp, #0x60]
>
> x9 is volatile in the AAPCS64 ABI, so at this offset in the assembly the
> value has already been overwritten by the call instruction at +84.  If I
> later see a load of x9 and mark the register as "IsSame", now we've got a
> problem because we're saying it has the original value.
>
> If we were going to follow the IsSame-means-unmodified thinking through,
> we'd want to mark every register as IsSame on function entry and only
> remove that markup when the register is modified.
>
> I guess I'm trying to say two things.  (1) I'd really like to get rid of
> IsSame so that the unwind plan dumps are easier for me to read ;) and (2) I
> think the instruction profiler unwind plan generators should track all
> registers without knowledge of the ABI, and leave it to the runtime code to
> decide which registers we will allow to be looked up.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D26295
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20161108/ef83e056/attachment-0001.html>