diff mbox series

gdb/riscv: add systemtap support

Message ID b063d1818331f944d129e62aff91b22a643679f1.1679300298.git.aburgess@redhat.com
State New
Headers
Series gdb/riscv: add systemtap support |

Commit Message

Andrew Burgess March 20, 2023, 8:18 a.m. UTC 
  This commit is initial support for SystemTap for RISC-V Linux.  The
following two tests exercise SystemTap functionality, and are showing
many failures, which are all fixed by this commit:

  gdb.cp/exceptprint.exp
  gdb.base/stap-probe.exp

One thing I wasn't sure about is if the SystemTap support should be
Linux specific, or architecture specific.  For aarch64, arm, ia64, and
ppc, the SystemTap support seems to libe in the ARCH-linux-tdep.c
file, while for amd64, i386, and s390 the implementation lives in
ARCH-tdep.c.  I have no idea which of these is the better choice -- or
maybe both choices are correct in the right circumstances, and I'm
just not aware of how to choose between them.

Anyway, for this patch I selected riscv-linux-tdep.c (though clearly,
moving the changes to riscv-tdep.c is trivial if anyone can why that's
a more appropriate location).  It makes sense to me that SystemTap
might be a Linux only tool, which is why I picked the Linux tdep file.

The stap-probe.exp file tests immediate, register, and register
indirect operands, all of which appear to be working fine with this
commit.  The generic expression support doesn't appear to be
architecture specific, so I'd expect that to work fine too.
---
 gdb/riscv-linux-tdep.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)


base-commit: 57d67e5883732ec5bc14f02c312d2000d75c1a10

Comments

Tom Tromey March 20, 2023, 1:43 p.m. UTC | #1 
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> One thing I wasn't sure about is if the SystemTap support should be
Andrew> Linux specific, or architecture specific.  For aarch64, arm, ia64, and
Andrew> ppc, the SystemTap support seems to libe in the ARCH-linux-tdep.c
Andrew> file, while for amd64, i386, and s390 the implementation lives in
Andrew> ARCH-tdep.c.  I have no idea which of these is the better choice -- or
Andrew> maybe both choices are correct in the right circumstances, and I'm
Andrew> just not aware of how to choose between them.

It's really an ELF feature, but in practice AFAIK it is only used on
Linux.  Putting it in the arch tdep file seems fine to me, but nothing
bad will happen if it is in the linux-tdep file.

Andrew> Anyway, for this patch I selected riscv-linux-tdep.c (though clearly,
Andrew> moving the changes to riscv-tdep.c is trivial if anyone can why that's
Andrew> a more appropriate location).  It makes sense to me that SystemTap
Andrew> might be a Linux only tool, which is why I picked the Linux tdep file.

Yeah, they are called SystemTap probes but really they are just
implemented as a single header file that could be used elsewhere.

Tom
Andrew Burgess March 20, 2023, 8:47 p.m. UTC | #2 
Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> One thing I wasn't sure about is if the SystemTap support should be
> Andrew> Linux specific, or architecture specific.  For aarch64, arm, ia64, and
> Andrew> ppc, the SystemTap support seems to libe in the ARCH-linux-tdep.c
> Andrew> file, while for amd64, i386, and s390 the implementation lives in
> Andrew> ARCH-tdep.c.  I have no idea which of these is the better choice -- or
> Andrew> maybe both choices are correct in the right circumstances, and I'm
> Andrew> just not aware of how to choose between them.
>
> It's really an ELF feature, but in practice AFAIK it is only used on
> Linux.  Putting it in the arch tdep file seems fine to me, but nothing
> bad will happen if it is in the linux-tdep file.

Thanks for the insight.  I'll move the changes over to riscv-tdep.c and
update the commit message.

Thanks,
Andrew

>
> Andrew> Anyway, for this patch I selected riscv-linux-tdep.c (though clearly,
> Andrew> moving the changes to riscv-tdep.c is trivial if anyone can why that's
> Andrew> a more appropriate location).  It makes sense to me that SystemTap
> Andrew> might be a Linux only tool, which is why I picked the Linux tdep file.
>
> Yeah, they are called SystemTap probes but really they are just
> implemented as a single header file that could be used elsewhere.
>
> Tom
diff mbox series

Patch

diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
index 292d7a4ef7c..f1a8dbbb71e 100644
--- a/gdb/riscv-linux-tdep.c
+++ b/gdb/riscv-linux-tdep.c
@@ -26,6 +26,7 @@ 
 #include "tramp-frame.h"
 #include "trad-frame.h"
 #include "gdbarch.h"
+#include "safe-ctype.h"
 
 /* The following value is derived from __NR_rt_sigreturn in
    <include/uapi/asm-generic/unistd.h> from the Linux source tree.  */
@@ -174,6 +175,33 @@  riscv_linux_syscall_next_pc (frame_info_ptr frame)
   return pc + 4 /* Length of the ECALL insn.  */;
 }
 
+/* Implementation of `gdbarch_stap_is_single_operand', as defined in
+   gdbarch.h.  */
+
+static int
+riscv_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
+{
+  return (ISDIGIT (*s) /* Literal number.  */
+	  || *s == '(' /* Register indirection.  */
+	  || ISALPHA (*s)); /* Register value.  */
+}
+
+/* String that appears before a register name in a SystemTap register
+   indirect expression.  */
+
+static const char *const stap_register_indirection_prefixes[] =
+{
+  "(", nullptr
+};
+
+/* String that appears after a register name in a SystemTap register
+   indirect expression.  */
+
+static const char *const stap_register_indirection_suffixes[] =
+{
+  ")", nullptr
+};
+
 /* Initialize RISC-V Linux ABI info.  */
 
 static void
@@ -206,6 +234,13 @@  riscv_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   tramp_frame_prepend_unwinder (gdbarch, &riscv_linux_sigframe);
 
   tdep->syscall_next_pc = riscv_linux_syscall_next_pc;
+
+  /* SystemTap Support.  */
+  set_gdbarch_stap_is_single_operand (gdbarch, riscv_stap_is_single_operand);
+  set_gdbarch_stap_register_indirection_prefixes
+    (gdbarch, stap_register_indirection_prefixes);
+  set_gdbarch_stap_register_indirection_suffixes
+    (gdbarch, stap_register_indirection_suffixes);
 }
 
 /* Initialize RISC-V Linux target support.  */