[PATCH] [RISC-V]: Handling optimized prologue

[PATCH] [RISC-V]: Handling optimized prologue

[Lifang Xia]

From: Lifang Xia <lifang_xia@linux.alibaba.com>

The optimizer might shove anything into the prologue, if
we build up cache (cache != NULL) from scanning prologue,
we just skip what we don't recognize and scan further to
make cache as complete as possible.  However, if we skip
prologue, we'll stop immediately on unrecognized
instruction.

The case is:

----------------------------
Disassembly of section .text:

0000000000010078 <foo>:
   10078:       00100513                li      a0,1
   1007c:       008000ef                jal     ra,10084 <bar>
   10080:       00008067                ret

0000000000010084 <bar>:
   10084:       00051263                bnez    a0,10088 <bar+0x4>
   10088:       ff010113                addi    sp,sp,-16
   1008c:       00813023                sd      s0,0(sp)
   10090:       00050413                mv      s0,a0
   10094:       00113423                sd      ra,8(sp)
   10098:       014000ef                jal     ra,100ac <aaa>
   1009c:       00813083                ld      ra,8(sp)
   100a0:       00013403                ld      s0,0(sp)
   100a4:       01010113                addi    sp,sp,16
   100a8:       00008067                ret

00000000000100ac <aaa>:
   100ac:       00000013                nop
>> 100b0:       00008067                ret
----------------------
(gdb) bt
#0  0x00000000000100b0 in aaa ()
#1  0x000000000001009c in bar ()
Backtrace stopped: frame did not save the PC

gdb/
	* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not NULL.
---
 gdb/riscv-tdep.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 886996c..e46d441 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
       else
 	{
 	  end_prologue_addr = cur_pc;
-	  break;
+
+	  /* The optimizer might shove anything into the prologue, if
+	     we build up cache (cache != NULL) from scanning prologue,
+	     we just skip what we don't recognize and scan further to
+	     make cache as complete as possible.  However, if we skip
+	     prologue, we'll stop immediately on unrecognized
+	     instruction.  */
+	  if (cache == NULL)
+	    break;
 	}
     }
 
-- 
2.7.4

Re: [PATCH] [RISC-V]: Handling optimized prologue

[Andrew Burgess]

Lifang Xia <lifang_xia@c-sky.com> writes:

Hello Lifang,

Thanks for working on this.

> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>
> The optimizer might shove anything into the prologue, if
> we build up cache (cache != NULL) from scanning prologue,
> we just skip what we don't recognize and scan further to
> make cache as complete as possible.

My concern here would be, what if the thing you are skipping over
modifies the state in such a way, that when we think we are building the
cache, we're really doing the wrong thing in some way...


>                                      However, if we skip
> prologue, we'll stop immediately on unrecognized
> instruction.
>
> The case is:
>
> ----------------------------
> Disassembly of section .text:
>
> 0000000000010078 <foo>:
>    10078:       00100513                li      a0,1
>    1007c:       008000ef                jal     ra,10084 <bar>
>    10080:       00008067                ret
>
> 0000000000010084 <bar>:
>    10084:       00051263                bnez    a0,10088 <bar+0x4>
>    10088:       ff010113                addi    sp,sp,-16
>    1008c:       00813023                sd      s0,0(sp)
>    10090:       00050413                mv      s0,a0
>    10094:       00113423                sd      ra,8(sp)
>    10098:       014000ef                jal     ra,100ac <aaa>
>    1009c:       00813083                ld      ra,8(sp)
>    100a0:       00013403                ld      s0,0(sp)
>    100a4:       01010113                addi    sp,sp,16
>    100a8:       00008067                ret
>
> 00000000000100ac <aaa>:
>    100ac:       00000013                nop
>>> 100b0:       00008067                ret
> ----------------------
> (gdb) bt
> #0  0x00000000000100b0 in aaa ()
> #1  0x000000000001009c in bar ()
> Backtrace stopped: frame did not save the PC

I assume it's the bnez that's causing the problem for the prologue scan
here?

Not that I really understand what that instruction is about, it appears
to be branching to the next instruction.  I wonder if this objdump
output is for the object file, rather than the executable?  I'm guessing
the bnez has a reloc which points at, maybe, the ret at 0x100a8?

I wonder if a better solution in this case would be to allow the
prologue scan to skip over forward branches within the prologue, maybe
with the limitation that the destination has to be within the function
bounds?

>
> gdb/
> 	* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not NULL.
> ---
>  gdb/riscv-tdep.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 886996c..e46d441 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>        else
>  	{
>  	  end_prologue_addr = cur_pc;
> -	  break;
> +
> +	  /* The optimizer might shove anything into the prologue, if
> +	     we build up cache (cache != NULL) from scanning prologue,
> +	     we just skip what we don't recognize and scan further to
> +	     make cache as complete as possible.  However, if we skip
> +	     prologue, we'll stop immediately on unrecognized
> +	     instruction.  */
> +	  if (cache == NULL)

Use nullptr not NULL please.

Thanks,
Andrew



> +	    break;
>  	}
>      }
>  
> -- 
> 2.7.4

答复: [PATCH] [RISC-V]: Handling optimized prologue

[lifang_xia]

Hi Andrew,
Thanks for reviewing.
See below.


> -----邮件原件-----
> 发件人: Andrew Burgess <aburgess@redhat.com>
> 发送时间: 2022年3月1日 18:34
> 收件人: Lifang Xia <lifang_xia@c-sky.com>; gdb-patches@sourceware.org
> 抄送: Lifang Xia <lifang_xia@linux.alibaba.com>
> 主题: Re: [PATCH] [RISC-V]: Handling optimized prologue
> 
> Lifang Xia <lifang_xia@c-sky.com> writes:
> 
> Hello Lifang,
> 
> Thanks for working on this.
> 
> > From: Lifang Xia <lifang_xia@linux.alibaba.com>
> >
> > The optimizer might shove anything into the prologue, if we build up
> > cache (cache != NULL) from scanning prologue, we just skip what we
> > don't recognize and scan further to make cache as complete as
> > possible.
> 
> My concern here would be, what if the thing you are skipping over modifies
> the state in such a way, that when we think we are building the cache,
we're
> really doing the wrong thing in some way...
> 
> 
> >                                      However, if we skip prologue,
> > we'll stop immediately on unrecognized instruction.
> >
> > The case is:
> >
> > ----------------------------
> > Disassembly of section .text:
> >
> > 0000000000010078 <foo>:
> >    10078:       00100513                li      a0,1
> >    1007c:       008000ef                jal     ra,10084 <bar>
> >    10080:       00008067                ret
> >
> > 0000000000010084 <bar>:
> >    10084:       00051263                bnez    a0,10088 <bar+0x4>
> >    10088:       ff010113                addi    sp,sp,-16
> >    1008c:       00813023                sd      s0,0(sp)
> >    10090:       00050413                mv      s0,a0
> >    10094:       00113423                sd      ra,8(sp)
> >    10098:       014000ef                jal     ra,100ac <aaa>
> >    1009c:       00813083                ld      ra,8(sp)
> >    100a0:       00013403                ld      s0,0(sp)
> >    100a4:       01010113                addi    sp,sp,16
> >    100a8:       00008067                ret
> >
> > 00000000000100ac <aaa>:
> >    100ac:       00000013                nop
> >>> 100b0:       00008067                ret
> > ----------------------
> > (gdb) bt
> > #0  0x00000000000100b0 in aaa ()
> > #1  0x000000000001009c in bar ()
> > Backtrace stopped: frame did not save the PC
> 
> I assume it's the bnez that's causing the problem for the prologue scan
here?
> 
> Not that I really understand what that instruction is about, it appears to
be
> branching to the next instruction.  I wonder if this objdump output is for
the
> object file, rather than the executable?  I'm guessing the bnez has a
reloc
> which points at, maybe, the ret at 0x100a8?
> 
> I wonder if a better solution in this case would be to allow the prologue
scan
> to skip over forward branches within the prologue, maybe with the
limitation
> that the destination has to be within the function bounds?
> 

Yes, You are right. bnez cause the prologue scan failed.
It's one of the cases I have met. As mentioned before, The optimizer of
compiler might shove anything to the prologue.
It could be multiple instructions, shift instructions, condition branches
...
We can't handle all of instructions from  the ISA specs or vendor extensions
here.

Actually, if cache is not nullptr, that means we need to build the frame
information from prologue. Before the  loop, we get the base address and the
prologue_end,
 traversing all the prologue is not a bad choice to build the frame(if cache
is not nullptr). 

> >
> > gdb/
> > 	* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not
> NULL.
> > ---
> >  gdb/riscv-tdep.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index
> > 886996c..e46d441 100644
> > --- a/gdb/riscv-tdep.c
> > +++ b/gdb/riscv-tdep.c
> > @@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> >        else
> >  	{
> >  	  end_prologue_addr = cur_pc;
> > -	  break;
> > +
> > +	  /* The optimizer might shove anything into the prologue, if
> > +	     we build up cache (cache != NULL) from scanning prologue,
> > +	     we just skip what we don't recognize and scan further to
> > +	     make cache as complete as possible.  However, if we skip
> > +	     prologue, we'll stop immediately on unrecognized
> > +	     instruction.  */
> > +	  if (cache == NULL)
> 
> Use nullptr not NULL please.
> 
> Thanks,
> Andrew
> 
> 
> 
> > +	    break;
> >  	}
> >      }
> >
> > --
> > 2.7.4

Re: 答复: [PATCH] [RISC-V]: Handling optimized prologue

[Andrew Burgess]

lifang_xia--- via Gdb-patches <gdb-patches@sourceware.org> writes:

> Hi Andrew,
> Thanks for reviewing.
> See below.
>
>
>> -----邮件原件-----
>> 发件人: Andrew Burgess <aburgess@redhat.com>
>> 发送时间: 2022年3月1日 18:34
>> 收件人: Lifang Xia <lifang_xia@c-sky.com>; gdb-patches@sourceware.org
>> 抄送: Lifang Xia <lifang_xia@linux.alibaba.com>
>> 主题: Re: [PATCH] [RISC-V]: Handling optimized prologue
>> 
>> Lifang Xia <lifang_xia@c-sky.com> writes:
>> 
>> Hello Lifang,
>> 
>> Thanks for working on this.
>> 
>> > From: Lifang Xia <lifang_xia@linux.alibaba.com>
>> >
>> > The optimizer might shove anything into the prologue, if we build up
>> > cache (cache != NULL) from scanning prologue, we just skip what we
>> > don't recognize and scan further to make cache as complete as
>> > possible.
>> 
>> My concern here would be, what if the thing you are skipping over modifies
>> the state in such a way, that when we think we are building the cache,
> we're
>> really doing the wrong thing in some way...
>> 
>> 
>> >                                      However, if we skip prologue,
>> > we'll stop immediately on unrecognized instruction.
>> >
>> > The case is:
>> >
>> > ----------------------------
>> > Disassembly of section .text:
>> >
>> > 0000000000010078 <foo>:
>> >    10078:       00100513                li      a0,1
>> >    1007c:       008000ef                jal     ra,10084 <bar>
>> >    10080:       00008067                ret
>> >
>> > 0000000000010084 <bar>:
>> >    10084:       00051263                bnez    a0,10088 <bar+0x4>
>> >    10088:       ff010113                addi    sp,sp,-16
>> >    1008c:       00813023                sd      s0,0(sp)
>> >    10090:       00050413                mv      s0,a0
>> >    10094:       00113423                sd      ra,8(sp)
>> >    10098:       014000ef                jal     ra,100ac <aaa>
>> >    1009c:       00813083                ld      ra,8(sp)
>> >    100a0:       00013403                ld      s0,0(sp)
>> >    100a4:       01010113                addi    sp,sp,16
>> >    100a8:       00008067                ret
>> >
>> > 00000000000100ac <aaa>:
>> >    100ac:       00000013                nop
>> >>> 100b0:       00008067                ret
>> > ----------------------
>> > (gdb) bt
>> > #0  0x00000000000100b0 in aaa ()
>> > #1  0x000000000001009c in bar ()
>> > Backtrace stopped: frame did not save the PC
>> 
>> I assume it's the bnez that's causing the problem for the prologue scan
> here?
>> 
>> Not that I really understand what that instruction is about, it appears to
> be
>> branching to the next instruction.  I wonder if this objdump output is for
> the
>> object file, rather than the executable?  I'm guessing the bnez has a
> reloc
>> which points at, maybe, the ret at 0x100a8?
>> 
>> I wonder if a better solution in this case would be to allow the prologue
> scan
>> to skip over forward branches within the prologue, maybe with the
> limitation
>> that the destination has to be within the function bounds?
>> 
>
> Yes, You are right. bnez cause the prologue scan failed.
> It's one of the cases I have met. As mentioned before, The optimizer of
> compiler might shove anything to the prologue.
> It could be multiple instructions, shift instructions, condition branches
> ...
> We can't handle all of instructions from  the ISA specs or vendor extensions
> here.
>
> Actually, if cache is not nullptr, that means we need to build the frame
> information from prologue. Before the  loop, we get the base address and the
> prologue_end,
>  traversing all the prologue is not a bad choice to build the frame(if cache
> is not nullptr).

Except right now, if we declare that we know something based on the
prologue then we can be reasonably sure that what we claim to know is
true.

Under your proposal we would claim to know things for which we actually
have no knowledge of whether it is true or not, e.g. if some previously
unknown instruction has adjusted the contents of a register in some way,
but we ignore the adjustment and just claim to know the previous
register contents anyway.

This then raises the question, would we prefer GDB to say "I don't
know", if it's not sure, or say "The answer is ....", when that might
not be true?

Are there other targets that do this aggressive prologue scanning?

An alternative might be to offer a new setting, which is off by default,
but which allows for the aggressive prologue scanning you suggest.
However, I think such an option would have to include a suitable warning
to indicate that some prologue instructions might be ignored.

I still think you should consider just adding support for forward
branches to the prologue scanner.  Though any instruction _could_ be
moved into the prologue, I suspect (guess) the number that actually _do_
get moved up is pretty small.

A different strategy might be to handle unknown instructions based on
their instruction class (R, I, S, etc).  If the instruction is only
touching registers that we don't need to preserve over the unwind, or is
touching registers for which the previous value has already been saved,
then maybe we don't need to care about the actual meaning of the
instruction, we can just mark the destination register as unknown and
move on.  I think this sounds much closer to what you want.

Thanks,
Andrew



>
>> >
>> > gdb/
>> > 	* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not
>> NULL.
>> > ---
>> >  gdb/riscv-tdep.c | 10 +++++++++-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index
>> > 886996c..e46d441 100644
>> > --- a/gdb/riscv-tdep.c
>> > +++ b/gdb/riscv-tdep.c
>> > @@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>> >        else
>> >  	{
>> >  	  end_prologue_addr = cur_pc;
>> > -	  break;
>> > +
>> > +	  /* The optimizer might shove anything into the prologue, if
>> > +	     we build up cache (cache != NULL) from scanning prologue,
>> > +	     we just skip what we don't recognize and scan further to
>> > +	     make cache as complete as possible.  However, if we skip
>> > +	     prologue, we'll stop immediately on unrecognized
>> > +	     instruction.  */
>> > +	  if (cache == NULL)
>> 
>> Use nullptr not NULL please.
>> 
>> Thanks,
>> Andrew
>> 
>> 
>> 
>> > +	    break;
>> >  	}
>> >      }
>> >
>> > --
>> > 2.7.4

答复: 答复: [PATCH] [RISC-V]: Handling optimized prologue

[lifang_xia]

> >>
> >> I assume it's the bnez that's causing the problem for the prologue
> >> scan
> > here?
> >>
> >> Not that I really understand what that instruction is about, it
> >> appears to
> > be
> >> branching to the next instruction.  I wonder if this objdump output
> >> is for
> > the
> >> object file, rather than the executable?  I'm guessing the bnez has a
> > reloc
> >> which points at, maybe, the ret at 0x100a8?
> >>
> >> I wonder if a better solution in this case would be to allow the
> >> prologue
> > scan
> >> to skip over forward branches within the prologue, maybe with the
> > limitation
> >> that the destination has to be within the function bounds?
> >>
> >
> > Yes, You are right. bnez cause the prologue scan failed.
> > It's one of the cases I have met. As mentioned before, The optimizer
> > of compiler might shove anything to the prologue.
> > It could be multiple instructions, shift instructions, condition
> > branches ...
> > We can't handle all of instructions from  the ISA specs or vendor
> > extensions here.
> >
> > Actually, if cache is not nullptr, that means we need to build the
> > frame information from prologue. Before the  loop, we get the base
> > address and the prologue_end,  traversing all the prologue is not a
> > bad choice to build the frame(if cache is not nullptr).
> 
> Except right now, if we declare that we know something based on the prologue
> then we can be reasonably sure that what we claim to know is true.
> 
> Under your proposal we would claim to know things for which we actually have
> no knowledge of whether it is true or not, e.g. if some previously unknown
> instruction has adjusted the contents of a register in some way, but we ignore
> the adjustment and just claim to know the previous register contents anyway.
> 
> This then raises the question, would we prefer GDB to say "I don't know", if it's
> not sure, or say "The answer is ....", when that might not be true?
> 
> Are there other targets that do this aggressive prologue scanning?

At first, my idea was that mark the action of storing RA as a flag_could_be_stop and we can stop the scanning  if flag_could_be_stop  or  "cache is nullptr".
But it still not a best choice, it might lost some registers' value which are saved in this frame.
When I saw ARM/NDS32 do this, and I followed them. 

> 
> An alternative might be to offer a new setting, which is off by default, but which
> allows for the aggressive prologue scanning you suggest.
> However, I think such an option would have to include a suitable warning to
> indicate that some prologue instructions might be ignored.

It's not a good choice. This phenomenon occurs frequently when we debug an optimized program.

> 
> I still think you should consider just adding support for forward branches to the
> prologue scanner.  Though any instruction _could_ be moved into the prologue,
> I suspect (guess) the number that actually _do_ get moved up is pretty small.

I thought about doing this, but after discussing it with my colleagues who maintaining the compilers.
There are many instructions that may appear here, and it may not be possible to list them all.

> 
> A different strategy might be to handle unknown instructions based on their
> instruction class (R, I, S, etc).  If the instruction is only touching registers that we
> don't need to preserve over the unwind, or is touching registers for which the
> previous value has already been saved, then maybe we don't need to care about
> the actual meaning of the instruction, we can just mark the destination register
> as unknown and move on.  I think this sounds much closer to what you want.

I can try to find  a way to solve it with your idea. But it may take a few of days.
 
Thanks a lot,
Lifang
> 
> Thanks,
> Andrew
> 
> 
> 
> >
> >> >
> >> > gdb/
> >> > 	* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not
> >> NULL.
> >> > ---
> >> >  gdb/riscv-tdep.c | 10 +++++++++-
> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index
> >> > 886996c..e46d441 100644
> >> > --- a/gdb/riscv-tdep.c
> >> > +++ b/gdb/riscv-tdep.c
> >> > @@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> >> >        else
> >> >  	{
> >> >  	  end_prologue_addr = cur_pc;
> >> > -	  break;
> >> > +
> >> > +	  /* The optimizer might shove anything into the prologue, if
> >> > +	     we build up cache (cache != NULL) from scanning prologue,
> >> > +	     we just skip what we don't recognize and scan further to
> >> > +	     make cache as complete as possible.  However, if we skip
> >> > +	     prologue, we'll stop immediately on unrecognized
> >> > +	     instruction.  */
> >> > +	  if (cache == NULL)
> >>
> >> Use nullptr not NULL please.
> >>
> >> Thanks,
> >> Andrew
> >>
> >>
> >>
> >> > +	    break;
> >> >  	}
> >> >      }
> >> >
> >> > --
> >> > 2.7.4