[PATCH] gas: Improve documentation for cfi_remember/restore_state

[PATCH] gas: Improve documentation for cfi_remember/restore_state

[Martin Galvan]

The previous documentation for these CFI directives wasn't very helpful, so I
reworded it and added an example. Any feedback is welcome.

I have a company-wide copyright assignment for binutils, but I don't have write
access so someone else should commit this for me. Thanks!

gas/ChangeLog:
2016-04-13  Martin Galvan  <martin.galvan@tallertechnologies.com>

	* doc/as.texinfo (.cfi_remember_state, .cfi_restore_state): Improve
	documentation.
---
 gas/doc/as.texinfo | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/gas/doc/as.texinfo b/gas/doc/as.texinfo
index 7b36931..01fc17d 100644
--- a/gas/doc/as.texinfo
+++ b/gas/doc/as.texinfo
@@ -4816,11 +4816,39 @@ From now on the previous value of @var{register} can't be restored anymore.
 Current value of @var{register} is the same like in the previous frame,
 i.e. no restoration needed.
 
-@subsection @code{.cfi_remember_state},
-First save all current rules for all registers by @code{.cfi_remember_state},
-then totally screw them up by subsequent @code{.cfi_*} directives and when
-everything is hopelessly bad, use @code{.cfi_restore_state} to restore
-the previous saved state.
+@subsection @code{.cfi_remember_state} and @code{.cfi_restore_state}
+@code{.cfi_remember_state} pushes the set of rules for every register onto an
+implicit stack, while @code{.cfi_restore_state} pops them off the stack and
+places them in the current row.  This is useful for situations where you have
+@code{.cfi_*} directives that alter the following rows only under certain
+conditions.  For example, we could have something like this:
+
+@smallexample
+        jne  label
+        addq $42, %rsp
+        .cfi_adjust_cfa_offset -42
+        ret
+label:
+        /* Do something else */
+@end smallexample
+
+Here, we want the @code{.cfi} directive to affect only the row corresponding to
+the @code{ret} instruction, but not the code after @code{label}.  To do this we
+can write:
+
+@smallexample
+        .cfi_remember_state
+        jne  label
+        addq $42, %rsp
+        .cfi_adjust_cfa_offset -42
+        ret
+label:
+        .cfi_restore_state
+        /* Do something else */
+@end smallexample
+
+That way, the rules for the instructions after @code{label} will be the same
+as before the @{.cfi_adjust_cfa_offset}.
 
 @subsection @code{.cfi_return_column @var{register}}
 Change return column @var{register}, i.e. the return address is either
-- 
2.8.1

Re: [PATCH] gas: Improve documentation for cfi_remember/restore_state

[Alan Modra]

Thanks for doing this.

On Wed, Apr 13, 2016 at 06:49:20PM -0300, Martin Galvan wrote:
> diff --git a/gas/doc/as.texinfo b/gas/doc/as.texinfo
> index 7b36931..01fc17d 100644
> --- a/gas/doc/as.texinfo
> +++ b/gas/doc/as.texinfo
> @@ -4816,11 +4816,39 @@ From now on the previous value of @var{register} can't be restored anymore.
>  Current value of @var{register} is the same like in the previous frame,
>  i.e. no restoration needed.
>  
> -@subsection @code{.cfi_remember_state},
> -First save all current rules for all registers by @code{.cfi_remember_state},
> -then totally screw them up by subsequent @code{.cfi_*} directives and when
> -everything is hopelessly bad, use @code{.cfi_restore_state} to restore
> -the previous saved state.
> +@subsection @code{.cfi_remember_state} and @code{.cfi_restore_state}
> +@code{.cfi_remember_state} pushes the set of rules for every register onto an
> +implicit stack, while @code{.cfi_restore_state} pops them off the stack and
> +places them in the current row.  This is useful for situations where you have
> +@code{.cfi_*} directives that alter the following rows only under certain
> +conditions.  For example, we could have something like this:

Control flow in a program has no effect on interpretation of
debug/eh_frame info.  I think your wording here is giving the
impression that it does..

> +
> +@smallexample
> +        jne  label
> +        addq $42, %rsp
> +        .cfi_adjust_cfa_offset -42
> +        ret
> +label:
> +        /* Do something else */
> +@end smallexample
> +
> +Here, we want the @code{.cfi} directive to affect only the row corresponding to
> +the @code{ret} instruction, but not the code after @code{label}.  To do this we
> +can write:
> +
> +@smallexample
> +        .cfi_remember_state
> +        jne  label
> +        addq $42, %rsp
> +        .cfi_adjust_cfa_offset -42
> +        ret
> +label:
> +        .cfi_restore_state
> +        /* Do something else */
> +@end smallexample
> +
> +That way, the rules for the instructions after @code{label} will be the same
> +as before the @{.cfi_adjust_cfa_offset}.
>  
>  @subsection @code{.cfi_return_column @var{register}}
>  Change return column @var{register}, i.e. the return address is either

I have two suggestions for improving your example.  As I mentioned
before, control flow in a program is not relevant, so the
".cfi_remember_state" does not need to be placed before the "jne".
ie.

@smallexample
        jne  label
        addq $42, %rsp
        .cfi_remember_state
        .cfi_adjust_cfa_offset -42
        ret
label:
        .cfi_restore_state
        /* Do something else */
@end smallexample

works just as well, and encodes better (fewer advance_loc opcodes).

Secondly, this isn't a very good example of code that really needs
".cfi_remember_state".  You could solve the problem in your example
with

@smallexample
        jne  label
        addq $42, %rsp
        .cfi_adjust_cfa_offset -42
        ret
label:
        .cfi_adjust_cfa_offset 42
        /* Do something else */
@end smallexample

giving a DWARF "program" that is the same size but should execute a
little faster when unwinding.  Would you please update your example to
something that does benefit from using .cfi_remember_state?  The
simplest one I can think of is if your example restored a register or
two before adjusting sp, then you might want to describe that with
.cfi_restore.  That then would require a larger DWARF program to
recreate the register save locations if you wanted to do so "by hand"
and thus use of remember_state/restore_state is justified.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas: Improve documentation for cfi_remember/restore_state

[Martin Galvan]

On Thu, Apr 14, 2016 at 3:39 AM, Alan Modra <amodra@gmail.com> wrote:
> Control flow in a program has no effect on interpretation of
> debug/eh_frame info.  I think your wording here is giving the
> impression that it does..

Yeah, definitely. It's actually kind of tricky to explain, more so
when I last used this years ago :)

> giving a DWARF "program" that is the same size but should execute a
> little faster when unwinding.  Would you please update your example to
> something that does benefit from using .cfi_remember_state?  The
> simplest one I can think of is if your example restored a register or
> two before adjusting sp, then you might want to describe that with
> .cfi_restore.  That then would require a larger DWARF program to
> recreate the register save locations if you wanted to do so "by hand"
> and thus use of remember_state/restore_state is justified.

Yeah, I guess you're right. I actually based my example on something
generated by gcc -S.

You mean something like:

    je label
    popq %rbx
    .cfi_remember_state
    .cfi_restore %rbx
    popq %rbp
    .cfi_restore %rbp
    popq %r12
    .cfi_restore %r12
    ret

label:
    .cfi_restore_state
    /* Do something else */

In that case we're using .cfi_restore_state to save us having to use
multiple CFI directives to recreate the original save locations.

Re: [PATCH] gas: Improve documentation for cfi_remember/restore_state

[Alan Modra]

On Thu, Apr 14, 2016 at 11:23:08AM -0300, Martin Galvan wrote:
> You mean something like:
> 
>     je label
>     popq %rbx
>     .cfi_remember_state
>     .cfi_restore %rbx
>     popq %rbp
>     .cfi_restore %rbp
>     popq %r12
>     .cfi_restore %r12
>     ret
> 
> label:
>     .cfi_restore_state
>     /* Do something else */
> 
> In that case we're using .cfi_restore_state to save us having to use
> multiple CFI directives to recreate the original save locations.

Yes, exactly.  However the above example shows a gcc bug!  Presumably
the cfa is set to rbp, because if the cfa was rsp you'd need cfa
offset adjustment on each pop.  So when rbp is popped, cfa ought to be
set to rsp with an offset, and there be a cfa offset adjustment on the
pop of r12.  The bug would show up if an asynchronous interrupt was
taken after the pop of rbp, and the signal handler wanted to unwind
the stack for some reason.

Hmm, seems like current mainline gcc is buggy in this area on x86_64.
I see this sort of thing around a tail call:
        je      .L4
        popq    %rbp
        .cfi_remember_state
        .cfi_def_cfa 7, 8
        movl    $1, %edi
        jmp     *%rax
.L4:
        .cfi_restore_state
So the cfa is set back to rsp on popping rbp, but there ought to be a
".cfi_restore 6".  Otherwise when an async interrupt hits after the
pop of rbp, the unwinder will load rbp from the stack, which has just
been trashed by the interrupt handler..

It might be better to choose an example from gcc -fomit-frame-pointer
-fasynchronous-unwind-tables code.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas: Improve documentation for cfi_remember/restore_state

[Martin Galvan]

On Thu, Apr 14, 2016 at 8:16 PM, Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Apr 14, 2016 at 11:23:08AM -0300, Martin Galvan wrote:
> > You mean something like:
> >
> >     je label
> >     popq %rbx
> >     .cfi_remember_state
> >     .cfi_restore %rbx
> >     popq %rbp
> >     .cfi_restore %rbp
> >     popq %r12
> >     .cfi_restore %r12
> >     ret
> >
> > label:
> >     .cfi_restore_state
> >     /* Do something else */
> >
> > In that case we're using .cfi_restore_state to save us having to use
> > multiple CFI directives to recreate the original save locations.
>
> Yes, exactly.  However the above example shows a gcc bug!

If you're referring to the last example I sent (with the three pops),
I wrote that manually. So it's a programmer bug, not gcc's :)

> Hmm, seems like current mainline gcc is buggy in this area on x86_64.
> I see this sort of thing around a tail call:
>         je      .L4
>         popq    %rbp
>         .cfi_remember_state
>         .cfi_def_cfa 7, 8
>         movl    $1, %edi
>         jmp     *%rax
> .L4:
>         .cfi_restore_state
> So the cfa is set back to rsp on popping rbp, but there ought to be a
> ".cfi_restore 6".  Otherwise when an async interrupt hits after the
> pop of rbp, the unwinder will load rbp from the stack, which has just
> been trashed by the interrupt handler..

That's probably true, though. I can look into it a bit more if you
want. I know next to nothing about gcc internals, but a couple guys at
the office can give me a hand with it.

> It might be better to choose an example from gcc -fomit-frame-pointer
> -fasynchronous-unwind-tables code.

Could we keep my 3-pop example if I added the required CFA adjustment?
I'd like to keep the example as simple as possible for the
documentation.

Re: [PATCH] gas: Improve documentation for cfi_remember/restore_state

[Alan Modra]

On Fri, Apr 15, 2016 at 11:21:31AM -0300, Martin Galvan wrote:
> On Thu, Apr 14, 2016 at 8:16 PM, Alan Modra <amodra@gmail.com> wrote:
> >
> > On Thu, Apr 14, 2016 at 11:23:08AM -0300, Martin Galvan wrote:
> > > You mean something like:
> > >
> > >     je label
> > >     popq %rbx
> > >     .cfi_remember_state
> > >     .cfi_restore %rbx
> > >     popq %rbp
> > >     .cfi_restore %rbp
> > >     popq %r12
> > >     .cfi_restore %r12
> > >     ret
> > >
> > > label:
> > >     .cfi_restore_state
> > >     /* Do something else */
> > >
> > > In that case we're using .cfi_restore_state to save us having to use
> > > multiple CFI directives to recreate the original save locations.
> >
> > Yes, exactly.  However the above example shows a gcc bug!
> 
> If you're referring to the last example I sent (with the three pops),
> I wrote that manually. So it's a programmer bug, not gcc's :)

Ah, OK.

> > Hmm, seems like current mainline gcc is buggy in this area on x86_64.
> > I see this sort of thing around a tail call:
> >         je      .L4
> >         popq    %rbp
> >         .cfi_remember_state
> >         .cfi_def_cfa 7, 8
> >         movl    $1, %edi
> >         jmp     *%rax
> > .L4:
> >         .cfi_restore_state
> > So the cfa is set back to rsp on popping rbp, but there ought to be a
> > ".cfi_restore 6".  Otherwise when an async interrupt hits after the
> > pop of rbp, the unwinder will load rbp from the stack, which has just
> > been trashed by the interrupt handler..
> 
> That's probably true, though. I can look into it a bit more if you
> want. I know next to nothing about gcc internals, but a couple guys at
> the office can give me a hand with it.

I suspect there's no need to look into it yourself.  x86_64 doesn't
lack interested and capable developers.

> > It might be better to choose an example from gcc -fomit-frame-pointer
> > -fasynchronous-unwind-tables code.
> 
> Could we keep my 3-pop example if I added the required CFA adjustment?
> I'd like to keep the example as simple as possible for the
> documentation.

Yes.

-- 
Alan Modra
Australia Development Lab, IBM