Re: gdb for Riscv, single stepping issue

On 6/29/22 2:34 AM, Pedro Alves wrote:
> On 2022-06-28 16:52, John Baldwin wrote:
>> On 6/27/22 1:40 PM, James Becker wrote:
>>> Hello,
>>>
>>> I have a RISCV-EL2 core running in a Nexys A7 FPGA board.
>>>
>>> I have openocd for riscv running over jtag with a connection by
>>> riscv-gdb to the openocd instance at port 3333.
>>>
>>> Everything works fine, stepping, break points, load, view memory.
>>>
>>> But I have one issue: Some of the memory in my design is 4 byte
>>> aligned.  Its designed for fast instruction fetch, its known as ICCM.
>>>
>>> When I have code running in that memory, gdb still works fine for
>>> breakpoints, but it will not single step.
>>>
>>> Looking at the openocd debug files, it appears that gdb is attempting to
>>> do a 2 byte read as a part of the single stepping procedure.
>>>
>>> Since my memory does not support 2 byte reads or writes, this fails.
>>>
>>> Is there some way that gdb can be configured to not do any 2-byte word
>>> reads or writes during single stepping?  I can't seem to find any.
>>
>> Hmm, setting breakpoints tries to read 1 byte at a time unless you have
>> disabled compressed breakpoints.  It looks like as a local hack you could
>> change use-compressed-breakpoints to just read 4 bytes rather than 2
>> initially?  Perhaps this is upstreamable if you make it read 4 bytes if
>> the target address is 4 byte aligned and only fall back to reading 2 bytes
>> if it isn't?
>>
> 
> Is that from riscv_breakpoint_kind_from_pc?  That uses target_read_code,
> so I'd think that since it goes via the code cache, it would read a whole
> cache line at once, meaning 64 bytes (show code-cache, show dcache line-size).
> 
> OTOH, riscv_insn::fetch_instruction uses target_read_memory to read 2 bytes,
> so I wonder whether this is the access in question:
> 
>    /* All insns are at least 16 bits.  */
>    status = target_read_memory (addr, buf, 2);
>    if (status)
>      memory_error (TARGET_XFER_E_IO, addr);
> 
> Why is this using target_read_memory instead of target_read_code, though?
> If it did that, then the code cache would be involved here too, papering over
> the issue, presumably.
> 
> Curious that the Riscv stub behaves this way, though.  I mean, failing the
> access instead of reading in aligned 4 bytes chunks, and doing read-modify-write,
> if necessary, hiding the issue from GDB.  Even inf-ptrace.c handles unaligned
> reads/writes and shorter-than-word-sized reads/writes itself, like:
> 
> static ULONGEST
> inf_ptrace_peek_poke (ptid_t ptid, gdb_byte *readbuf,
> 		      const gdb_byte *writebuf,
> 		      ULONGEST addr, ULONGEST len)
> {
> ...
>    /* We transfer aligned words.  Thus align ADDR down to a word
>       boundary and determine how many bytes to skip at the
>       beginning.  */
> ...
>        /* Read the word, also when doing a partial word write.  */
>        if (readbuf != NULL || chunk < sizeof (PTRACE_TYPE_RET))
> 	{
> 
> 
> I guess this also affects the "x" command at least (e.g., "x/2b $pc"), since that
> doesn't use the code cache either.

Yes, I had reworked my e-mail part way through and I really meant to talk about
this code, not the breakpoint code.  I agree that target_read_code is probably
more correct in fetch_instruction and would in this case hide the odd behavior
of the stub:

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 69f2123dcdb..09b2599e958 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1661,7 +1661,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
    int instlen, status;
  
    /* All insns are at least 16 bits.  */
-  status = target_read_memory (addr, buf, 2);
+  status = target_read_code (addr, buf, 2);
    if (status)
      memory_error (TARGET_XFER_E_IO, addr);
  
@@ -1672,7 +1672,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
  
    if (instlen > 2)
      {
-      status = target_read_memory (addr + 2, buf + 2, instlen - 2);
+      status = target_read_code (addr + 2, buf + 2, instlen - 2);
        if (status)
  	memory_error (TARGET_XFER_E_IO, addr + 2);
      }


-- 
John Baldwin