RISC-V: decr_pc_after_break causing problems

RISC-V: decr_pc_after_break causing problems

[Jim Wilson]

The RISC-V port in the riscv-tdep.c file has
  set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));

The privileged architecture spec v1.10 states in section 3.2.1 that
the ebreak instruction causes the receiving privilege mode's epc
register to be set to the address of the ebreak instruction, not the
address of the following instruction.  So gdb should not be
decrementing from the pc after a breakpoint is hit.

It isn't clear why this code is even here, as it isn't present in the
original gdb port in the github riscv/riscv-binutils-gdb tree.

Curiously, there is a corresponding bug in the riscv linux kernel
sources, where it is adding 4 to the sepc in the breakpoint trap
handling code for no apparent reason.  This might be OK if this was a
4-byte breakpoint instruction, but is not OK if this is a 2-byte
breakpoint instruction.

In order to get compressed breakpoints working on a SiFive HiFive
Unleashed board running linux, I need both the gdb and the linux
kernel bugs fixed.  The 4-byte breakpoint instruction works OK now,
but is not safe to use in code compiled with compressed instructions.
A good example is in the shared library support where _dl_debug_state
is a 2-byte function located 2-bytes before _dl_debug_initialize, so
placing a 4-byte breakpoint at _dl_debug_state overwrites the first
two bytes of the first instruction of _dl_debug_initialize causing it
to segfault.

I can submit patches for gdb and the linux kernel, but it would be
useful to know why gdb is trying to subtract from the pc after a
break.  Maybe someone has a part that doesn't conform to the v1.10
privilege architecture spec?  I noticed that this epc == breakpoint
address is not stated in earlier versions of the spec, which makes
earlier spec versions potentially ambiguous.  If we need to support
parts that don't conform to v1.10 priv spec then that makes the fix
more complicated.  It isn't clear how gdb is supposed to detect
whether a part conforms or not.  Maybe we can add an option to turn
this decrementing on
or off?  Maybe a configure option to select whether it is on/off by default?

There is another problem here incidentally that there is an option to
turn on/off compressed breakpoints, but it doesn't affect the amount
we subtract from the pc, which means this option can't work as
currently written.  This problem goes away if we stop decrementing the
pc in gdb.  If we have to keep the code that decrements the pc for
some targets, then maybe we should just eliminate the option.  It
isn't safe to use 4-byte breakpoints in code with compressed
instructions anyways.  And there is no point in using 2-byte
breakpoints in code with no compressed instructions.

Jim

Re: RISC-V: decr_pc_after_break causing problems

[Tim Newsome]

As another point of reference, the gdb we use with OpenOCD does not have
the set_gdbarch_decr_pc_after_break line at all.

Tim

On Mon, Jun 25, 2018 at 7:54 PM, Jim Wilson <jimw@sifive.com> wrote:

> The RISC-V port in the riscv-tdep.c file has
>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>
> The privileged architecture spec v1.10 states in section 3.2.1 that
> the ebreak instruction causes the receiving privilege mode's epc
> register to be set to the address of the ebreak instruction, not the
> address of the following instruction.  So gdb should not be
> decrementing from the pc after a breakpoint is hit.
>
> It isn't clear why this code is even here, as it isn't present in the
> original gdb port in the github riscv/riscv-binutils-gdb tree.
>
> Curiously, there is a corresponding bug in the riscv linux kernel
> sources, where it is adding 4 to the sepc in the breakpoint trap
> handling code for no apparent reason.  This might be OK if this was a
> 4-byte breakpoint instruction, but is not OK if this is a 2-byte
> breakpoint instruction.
>
> In order to get compressed breakpoints working on a SiFive HiFive
> Unleashed board running linux, I need both the gdb and the linux
> kernel bugs fixed.  The 4-byte breakpoint instruction works OK now,
> but is not safe to use in code compiled with compressed instructions.
> A good example is in the shared library support where _dl_debug_state
> is a 2-byte function located 2-bytes before _dl_debug_initialize, so
> placing a 4-byte breakpoint at _dl_debug_state overwrites the first
> two bytes of the first instruction of _dl_debug_initialize causing it
> to segfault.
>
> I can submit patches for gdb and the linux kernel, but it would be
> useful to know why gdb is trying to subtract from the pc after a
> break.  Maybe someone has a part that doesn't conform to the v1.10
> privilege architecture spec?  I noticed that this epc == breakpoint
> address is not stated in earlier versions of the spec, which makes
> earlier spec versions potentially ambiguous.  If we need to support
> parts that don't conform to v1.10 priv spec then that makes the fix
> more complicated.  It isn't clear how gdb is supposed to detect
> whether a part conforms or not.  Maybe we can add an option to turn
> this decrementing on
> or off?  Maybe a configure option to select whether it is on/off by
> default?
>
> There is another problem here incidentally that there is an option to
> turn on/off compressed breakpoints, but it doesn't affect the amount
> we subtract from the pc, which means this option can't work as
> currently written.  This problem goes away if we stop decrementing the
> pc in gdb.  If we have to keep the code that decrements the pc for
> some targets, then maybe we should just eliminate the option.  It
> isn't safe to use 4-byte breakpoints in code with compressed
> instructions anyways.  And there is no point in using 2-byte
> breakpoints in code with no compressed instructions.
>
> Jim
>

Re: RISC-V: decr_pc_after_break causing problems

[Jim Wilson]

On Mon, Jun 25, 2018 at 7:54 PM, Jim Wilson <jimw@sifive.com> wrote:
> The RISC-V port in the riscv-tdep.c file has
>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));

I'm still hoping to get a response to this.  I need to make
coordinated fixes to both gdb and the linux kernel to get breakpoints
working correctly.

Jim

Re: RISC-V: decr_pc_after_break causing problems

[Palmer Dabbelt]

On Tue, 03 Jul 2018 17:17:04 PDT (-0700), Jim Wilson wrote:
> On Mon, Jun 25, 2018 at 7:54 PM, Jim Wilson <jimw@sifive.com> wrote:
>> The RISC-V port in the riscv-tdep.c file has
>>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>
> I'm still hoping to get a response to this.  I need to make
> coordinated fixes to both gdb and the linux kernel to get breakpoints
> working correctly.

Andrew: I think this materialized itself when you submitted the GDB patches, 
probably because we have this in our Linux code:

    asmlinkage void do_trap_break(struct pt_regs *regs)
    {
    #ifdef CONFIG_GENERIC_BUG
            if (!user_mode(regs)) {
                    enum bug_trap_type type;
    
                    type = report_bug(regs->sepc, regs);
                    switch (type) {
                    case BUG_TRAP_TYPE_NONE:
                            break;
                    case BUG_TRAP_TYPE_WARN:
                            regs->sepc += sizeof(bug_insn_t);
                            return;
                    case BUG_TRAP_TYPE_BUG:
                            die(regs, "Kernel BUG");
                    }
            }
    #endif /* CONFIG_GENERIC_BUG */
    
            force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc), current);
            regs->sepc += 0x4;
    }

There's at least one bug in the Linux port here: we can enter a breakpoint trap 
via either ebreak (a 4-byte instruction) or c.ebreak (a 2-byte instruction).  
c.ebreak is necessary for a sane debugger so we need to support it.  Our 
options are:

* Handle c.ebreak in Linux and leave this as it stands.
* Remove both the Linux PC adjustment and the GDB PC adjustment.

I'm inclined to take the second option as it's less code.  I suppose 
technically it's an ABI break, but since it's broken anyway then I'm happy with 
taking it.

Is there something I'm missing?  If not Jim will submit a Linux patch and then 
we'll pull the trigger on this one.

Re: RISC-V: decr_pc_after_break causing problems

[John Baldwin]

On 7/3/18 5:35 PM, Palmer Dabbelt wrote:
> On Tue, 03 Jul 2018 17:17:04 PDT (-0700), Jim Wilson wrote:
>> On Mon, Jun 25, 2018 at 7:54 PM, Jim Wilson <jimw@sifive.com> wrote:
>>> The RISC-V port in the riscv-tdep.c file has
>>>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>>
>> I'm still hoping to get a response to this.  I need to make
>> coordinated fixes to both gdb and the linux kernel to get breakpoints
>> working correctly.
> 
> Andrew: I think this materialized itself when you submitted the GDB patches, 
> probably because we have this in our Linux code:
> 
>     asmlinkage void do_trap_break(struct pt_regs *regs)
>     {
>     #ifdef CONFIG_GENERIC_BUG
>             if (!user_mode(regs)) {
>                     enum bug_trap_type type;
>     
>                     type = report_bug(regs->sepc, regs);
>                     switch (type) {
>                     case BUG_TRAP_TYPE_NONE:
>                             break;
>                     case BUG_TRAP_TYPE_WARN:
>                             regs->sepc += sizeof(bug_insn_t);
>                             return;
>                     case BUG_TRAP_TYPE_BUG:
>                             die(regs, "Kernel BUG");
>                     }
>             }
>     #endif /* CONFIG_GENERIC_BUG */
>     
>             force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc), current);
>             regs->sepc += 0x4;
>     }
> 
> There's at least one bug in the Linux port here: we can enter a breakpoint trap 
> via either ebreak (a 4-byte instruction) or c.ebreak (a 2-byte instruction).  
> c.ebreak is necessary for a sane debugger so we need to support it.  Our 
> options are:
> 
> * Handle c.ebreak in Linux and leave this as it stands.
> * Remove both the Linux PC adjustment and the GDB PC adjustment.
> 
> I'm inclined to take the second option as it's less code.  I suppose 
> technically it's an ABI break, but since it's broken anyway then I'm happy with 
> taking it.
> 
> Is there something I'm missing?  If not Jim will submit a Linux patch and then 
> we'll pull the trigger on this one.

FWIW, my preference would be for the decr_after_pc_break match the hardware
which from my understanding of the thread means it should always be zero.

-- 
John Baldwin

Re: RISC-V: decr_pc_after_break causing problems

[Pedro Alves]

On 07/05/2018 11:54 PM, John Baldwin wrote:

> FWIW, my preference would be for the decr_after_pc_break match the hardware
> which from my understanding of the thread means it should always be zero.

Mine too, FWIW.  PC adjustment is a source of trouble in ports that need it.

Thanks,
Pedro Alves

Re: RISC-V: decr_pc_after_break causing problems

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2018-07-05 15:54:32 -0700]:

> On 7/3/18 5:35 PM, Palmer Dabbelt wrote:
> > On Tue, 03 Jul 2018 17:17:04 PDT (-0700), Jim Wilson wrote:
> >> On Mon, Jun 25, 2018 at 7:54 PM, Jim Wilson <jimw@sifive.com> wrote:
> >>> The RISC-V port in the riscv-tdep.c file has
> >>>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
> >>
> >> I'm still hoping to get a response to this.  I need to make
> >> coordinated fixes to both gdb and the linux kernel to get breakpoints
> >> working correctly.
> > 
> > Andrew: I think this materialized itself when you submitted the GDB patches, 
> > probably because we have this in our Linux code:
> > 
> >     asmlinkage void do_trap_break(struct pt_regs *regs)
> >     {
> >     #ifdef CONFIG_GENERIC_BUG
> >             if (!user_mode(regs)) {
> >                     enum bug_trap_type type;
> >     
> >                     type = report_bug(regs->sepc, regs);
> >                     switch (type) {
> >                     case BUG_TRAP_TYPE_NONE:
> >                             break;
> >                     case BUG_TRAP_TYPE_WARN:
> >                             regs->sepc += sizeof(bug_insn_t);
> >                             return;
> >                     case BUG_TRAP_TYPE_BUG:
> >                             die(regs, "Kernel BUG");
> >                     }
> >             }
> >     #endif /* CONFIG_GENERIC_BUG */
> >     
> >             force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc), current);
> >             regs->sepc += 0x4;
> >     }
> > 
> > There's at least one bug in the Linux port here: we can enter a breakpoint trap 
> > via either ebreak (a 4-byte instruction) or c.ebreak (a 2-byte instruction).  
> > c.ebreak is necessary for a sane debugger so we need to support it.  Our 
> > options are:
> > 
> > * Handle c.ebreak in Linux and leave this as it stands.
> > * Remove both the Linux PC adjustment and the GDB PC adjustment.
> > 
> > I'm inclined to take the second option as it's less code.  I suppose 
> > technically it's an ABI break, but since it's broken anyway then I'm happy with 
> > taking it.
> > 
> > Is there something I'm missing?  If not Jim will submit a Linux patch and then 
> > we'll pull the trigger on this one.
> 
> FWIW, my preference would be for the decr_after_pc_break match the hardware
> which from my understanding of the thread means it should always be
> zero.

I agree this solution is the correct approach and GDB should be
changed.

Thanks,
Andrew

Re: RISC-V: decr_pc_after_break causing problems

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-06-25 19:54:14 -0700]:

> The RISC-V port in the riscv-tdep.c file has
>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
> 
> The privileged architecture spec v1.10 states in section 3.2.1 that
> the ebreak instruction causes the receiving privilege mode's epc
> register to be set to the address of the ebreak instruction, not the
> address of the following instruction.  So gdb should not be
> decrementing from the pc after a breakpoint is hit.

Maybe I'm going to look completely silly here, but....

....aren't epc and pc different registers?

The epc is really many register, right? one for each privilege level,
uepc, sepc, and mepc.

The spec is clear that when an ebreak or c.ebreak is hit the contents
of *epc are the contents of the breaking instruction, but nothing is
said about the contents of pc.

> It isn't clear why this code is even here, as it isn't present in the
> original gdb port in the github riscv/riscv-binutils-gdb tree.
> 
> Curiously, there is a corresponding bug in the riscv linux kernel
> sources, where it is adding 4 to the sepc in the breakpoint trap
> handling code for no apparent reason.  This might be OK if this was a
> 4-byte breakpoint instruction, but is not OK if this is a 2-byte
> breakpoint instruction.
> 
> In order to get compressed breakpoints working on a SiFive HiFive
> Unleashed board running linux, I need both the gdb and the linux
> kernel bugs fixed.  The 4-byte breakpoint instruction works OK now,
> but is not safe to use in code compiled with compressed instructions.
> A good example is in the shared library support where _dl_debug_state
> is a 2-byte function located 2-bytes before _dl_debug_initialize, so
> placing a 4-byte breakpoint at _dl_debug_state overwrites the first
> two bytes of the first instruction of _dl_debug_initialize causing it
> to segfault.
> 
> I can submit patches for gdb and the linux kernel, but it would be
> useful to know why gdb is trying to subtract from the pc after a
> break.  Maybe someone has a part that doesn't conform to the v1.10
> privilege architecture spec?  I noticed that this epc == breakpoint
> address is not stated in earlier versions of the spec, which makes
> earlier spec versions potentially ambiguous.  If we need to support
> parts that don't conform to v1.10 priv spec then that makes the fix
> more complicated.  It isn't clear how gdb is supposed to detect
> whether a part conforms or not.  Maybe we can add an option to turn
> this decrementing on
> or off?  Maybe a configure option to select whether it is on/off by default?
> 
> There is another problem here incidentally that there is an option to
> turn on/off compressed breakpoints, but it doesn't affect the amount
> we subtract from the pc, which means this option can't work as
> currently written.  This problem goes away if we stop decrementing the
> pc in gdb.  If we have to keep the code that decrements the pc for
> some targets, then maybe we should just eliminate the option.  It
> isn't safe to use 4-byte breakpoints in code with compressed
> instructions anyways.  And there is no point in using 2-byte
> breakpoints in code with no compressed instructions.

Assuming that my above observations are correct, then when we hit a
breakpoint *epc is defined, but pc is not, an implementation could do
anything, right?  The two obvious choices are not increment pc, or
increment pc.

Right now my understanding is that GDB doesn't have special support
for fetching the program counter from different places depending on
the current inferior state (stopped at breakpoint, not stopped at
breakpoint).  So, somehow the value in *epc needs to be passed back
into the pc register (I guess).

I'm not exactly sure at which point in the process this is supposed to
happen.... I'll have another look around and see if any other target
does something similar.

Thanks,
Andrew

Re: RISC-V: decr_pc_after_break causing problems

[Jim Wilson]

On Wed, Jul 11, 2018 at 10:46 AM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Jim Wilson <jimw@sifive.com> [2018-06-25 19:54:14 -0700]:
>
>> The RISC-V port in the riscv-tdep.c file has
>>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>>
>> The privileged architecture spec v1.10 states in section 3.2.1 that
>> the ebreak instruction causes the receiving privilege mode's epc
>> register to be set to the address of the ebreak instruction, not the
>> address of the following instruction.  So gdb should not be
>> decrementing from the pc after a breakpoint is hit.
>
> Maybe I'm going to look completely silly here, but....
>
> ....aren't epc and pc different registers?

When you take a trap in linux, pc gets copied to sepc, and pc is set
to the linux kernel trap handler code.  The trap handler then stores
sepc into the process context, as it is the application pc.  When gdb
uses ptrace to get the pc, it is given the sepc, because that is the
application pc value.  So as far as gdb is concerned, there is only
one program counter.  The kernel just happens to call it sepc instead
of pc because that is where the application pc lives in the kernel.
When the kernel returns from a trap, executing the sret instruction,
the sepc is copied back into the pc thus returning us to the original
user code.

In the linux kernel sources, in arch/riscv/include/asm/ptrace.h, it has
struct pt_regs {
        unsigned long sepc;
        unsigned long ra;
        unsigned long sp;
        unsigned long gp;
       ...
};
so the kernel stores the pc where x0 would go, because we don't need
to save x0, and calls it sepc.

> The epc is really many register, right? one for each privilege level,
> uepc, sepc, and mepc.

Yes, but since linux runs at supervisor level, there is only sepc that
is interesting in this case.

> The spec is clear that when an ebreak or c.ebreak is hit the contents
> of *epc are the contents of the breaking instruction, but nothing is
> said about the contents of pc.

pc gets set to the trap handler address specified in the trap vector
table.  For linux, the address of that is presumably held in the stvec
CSR, and then either you go to a default offset from stvec, or it is
vectored by the exception number, or whatever, I haven't really looked
at that code.

> Assuming that my above observations are correct, then when we hit a
> breakpoint *epc is defined, but pc is not, an implementation could do
> anything, right?  The two obvious choices are not increment pc, or
> increment pc.

No, the pc has to be the address of a trap handler in the kernel,
because we took a trap.

> Right now my understanding is that GDB doesn't have special support
> for fetching the program counter from different places depending on
> the current inferior state (stopped at breakpoint, not stopped at
> breakpoint).  So, somehow the value in *epc needs to be passed back
> into the pc register (I guess).

The kernel takes care of sepc.  Gdb only needs to worry about the pc.
At least, that is how it works when running an OS like linux.  I don't
know how this is going to work in an embedded system.  Perhaps it is
OpenOCD that takes care of mapping the appropriate xepc to pc
depending on processor mode, and maybe OpenOCD also sets the
appropriate xtvec register to point at its trap handler code.

At no point in this process do we need to add or subtract anything from the pc.

Jim

Re: RISC-V: decr_pc_after_break causing problems

[Jim Wilson]

On Wed, Jul 11, 2018 at 11:20 AM, Jim Wilson <jimw@sifive.com> wrote:
> On Wed, Jul 11, 2018 at 10:46 AM, Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
>> * Jim Wilson <jimw@sifive.com> [2018-06-25 19:54:14 -0700]:
>>
>>> The RISC-V port in the riscv-tdep.c file has
>>>   set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));

Since everyone seems to agree that gdb should not be modifying pc if
it is unnecessary, I went ahead and submitted the linux kernel patch
side of this.
https://patchwork.kernel.org/patch/10524925/

We now need a gdb change to remove the code that calls
set_gdbarch_decr_pc_after_break, the has_compressed_isa local, and the
now incorrect comment in riscv_has_feature.  I can submit a patch for
that.

Jim