RE: Question: [PATCH] Change calculation of frame_id by amd64 epilogue unwinder

[Carl Love <cel@us.ibm.com>]

Bruno:
> 
> The idea of the patch looks fine, I just have a few style nits
> inlined.
> 
> > ---
> >   .../gdb.base/unwind-on-each-insn.exp          | 25
> > +++++++++++++++++--
> >   1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > index 3b48805cff8..c908a4b838e 100644
> > --- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > @@ -41,7 +41,6 @@ if ![runto_main] then {
> >   proc get_sp_and_fba { testname } {
> >       with_test_prefix "get \$sp and frame base $testname" {
> >   	set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> > -
> >   	set fba ""
> >   	gdb_test_multiple "info frame" "" {
> >   	    -re -wrap ".*Stack level ${::decimal}, frame at
> > ($::hex):.*" {
> > @@ -75,6 +74,23 @@ gdb_continue_to_breakpoint "enter foo"
> >   
> >   # Figure out the range of addresses covered by this function.
> >   set last_addr_in_foo ""
> > +
> > +# The disassembly of foo on PowerPC looks like:
> > +#     Dump of assembler code for function foo:
> > +#  => 0x00000000100006dc <+0>:     std     r31,-8(r1)
> > +#     0x00000000100006e0 <+4>:     stdu    r1,-48(r1)
> > +#     0x00000000100006e4 <+8>:     mr      r31,r1
> > +#     0x00000000100006e8 <+12>:    nop
> > +#     0x00000000100006ec <+16>:    addi    r1,r31,48
> > +#     0x00000000100006f0 <+20>:    ld      r31,-8(r1)
> > +#     0x00000000100006f4 <+24>:    blr
> > +#     0x00000000100006f8 <+28>:    .long 0x0
> > +#     0x00000000100006fc <+32>:    .long 0x0
> > +#     0x0000000010000700 <+36>:    .long 0x1000180
> > +#     End of assembler dump.
> > +#
> > +# The last instruction in function foo is blr.  Ignore the .long
> > +# entries following the blr instruction.
> >   gdb_test_multiple "disassemble foo" "" {
> >       -re "^disassemble foo\r\n" {
> >   	exp_continue
> > @@ -84,7 +100,11 @@ gdb_test_multiple "disassemble foo" "" {
> >   	exp_continue
> >       }
> >   
> > -    -re "^...($hex) \[^\r\n\]+\r\n" {
> > +    -re "^...($hex) \[<>+0-9:\s\t\]*\.long\[\s\t\]*\[^\r\n\]*\r\n" 
> > {
> 
> I wonder if this pattern is unnecessarily strict. Correct me if I'm 
> wrong, but I think that no architecture has an instruction that
> starts 
> with a .  so this pattern could probably be simplified to
> 
> ^...($hex) \[<>+0-9:\s\t\]*\.[^\r\n\]*\r\n

> 
> And no other architectures would have a similar problem in the
> future.
> 
> I'm not very knowledgeable on assembly so you may know better in this
> area.

I do not know of any architectures that have a dot at the beginning of
an instruction name.  So yes the pattern could be changed as suggested.
That will require changing the above comment as well to make it clear
we are skipping any "instruction" that starts with a dot, such as
.long.  
> 
> > +	exp_continue
> > +    }
> > +
> > +    -re "^...($hex)\[^\r\n\]+\r\n" {
> You removed a space here, which makes no difference in the pattern
> and 
> makes the diff more confusing

My bad, that was not intentional. I was too focused on the above change
and didn't notice the accidental dings to the code.  Sorry.

> >   	set last_addr_in_foo $expect_out(1,string)
> >   	exp_continue
> >       }
> > @@ -137,6 +157,7 @@ for { set i_count 1 } { true } { incr i_count }
> > {
> >   	gdb_test "frame 0" ".*"
> >   
> >   	set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
> > +
> Unnecessary new line here.
Ditto, unintentional change

I will undo the unintentional changes and update the needed
pattern/comment and post the patch for review.  Thanks for the help.

               Carl