[PATCH 0/1] riscv: Ensure LE instruction fetching

[PATCH 0/1] riscv: Ensure LE instruction fetching

[Branislav Brzak]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

Currently riscv gdb code looks at arch byte order
when fetching instructions. This works when the
target is LE, but on BE arch it will byte swap the
instruction, while the riscv spec defines all
instructions are LE encoded regardless of
system memory endianess.

Branislav Brzak (1):
gdb/riscv: Ensure LE instruction fetching

gdb/riscv-tdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Branislav Brzak]

[-- Attachment #1.1: Type: text/plain, Size: 735 bytes --]


________________________________
From: Branislav Brzak
Sent: Tuesday, June 20, 2023 1:46 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Dragoslav Sicarov <Dragoslav.Sicarov@Syrmia.com>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>
Subject: [PATCH 0/1] riscv: Ensure LE instruction fetching

Currently riscv gdb code looks at arch byte order
when fetching instructions. This works when the
target is LE, but on BE arch it will byte swap the
instruction, while the riscv spec defines all
instructions are LE encoded regardless of
system memory endianess.

Branislav Brzak (1):
gdb/riscv: Ensure LE instruction fetching

gdb/riscv-tdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-riscv-tdep.c-riscv_insn-fetch_instruction-Always-fet.patch --]
[-- Type: text/x-patch; name="0001-riscv-tdep.c-riscv_insn-fetch_instruction-Always-fet.patch", Size: 794 bytes --]

From 391716c3138697a0b8a1836c1ffdb44b1b6b9da4 Mon Sep 17 00:00:00 2001
From: Branislav Brzak <branislav.brzak@syrmia.com>
Date: Tue, 20 Jun 2023 13:20:40 +0200
Subject: [PATCH] * riscv-tdep.c (riscv_insn::fetch_instruction): Always fetch
 instructions as LE

---
 gdb/riscv-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 500279e1ae9..b4f98089937 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1812,7 +1812,7 @@ ULONGEST
 riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
+  enum bfd_endian byte_order = BFD_ENDIAN_LITTLE;
   gdb_byte buf[RISCV_MAX_INSN_LEN];
   int instlen, status;
 
-- 
2.34.1

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Tom Tromey]

> Currently riscv gdb code looks at arch byte order
> when fetching instructions. This works when the
> target is LE, but on BE arch it will byte swap the
> instruction, while the riscv spec defines all
> instructions are LE encoded regardless of
> system memory endianess.

Thank you for the patch.

> @@ -1812,7 +1812,7 @@ ULONGEST
>  riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
>  			       CORE_ADDR addr, int *len)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> +  enum bfd_endian byte_order = BFD_ENDIAN_LITTLE;

The variable is only used once, so you might as well remove it entirely
and replace it BFD_ENDIAN_LITTLE in the call.

A comment saying that instructions are always little-endian might be nice.

Tom

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Branislav Brzak]

[-- Attachment #1.1: Type: text/plain, Size: 1275 bytes --]

Hi Tom,

Here is the patch with the changes you specified.

Regards,
Branislav

________________________________
From: Tom Tromey <tom@tromey.com>
Sent: Tuesday, June 20, 2023 4:08 PM
To: Branislav Brzak <Branislav.Brzak@Syrmia.com>
Cc: gdb-patches@sourceware.org <gdb-patches@sourceware.org>; Dragoslav Sicarov <Dragoslav.Sicarov@Syrmia.com>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>
Subject: Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

> Currently riscv gdb code looks at arch byte order
> when fetching instructions. This works when the
> target is LE, but on BE arch it will byte swap the
> instruction, while the riscv spec defines all
> instructions are LE encoded regardless of
> system memory endianess.

Thank you for the patch.

> @@ -1812,7 +1812,7 @@ ULONGEST
>  riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
>                               CORE_ADDR addr, int *len)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> +  enum bfd_endian byte_order = BFD_ENDIAN_LITTLE;

The variable is only used once, so you might as well remove it entirely
and replace it BFD_ENDIAN_LITTLE in the call.

A comment saying that instructions are always little-endian might be nice.

Tom

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-riscv-tdep.c-riscv_insn-fetch_instruction-Always-fet.patch --]
[-- Type: text/x-patch; name="0001-riscv-tdep.c-riscv_insn-fetch_instruction-Always-fet.patch", Size: 1168 bytes --]

From 368fd34e4736192f053de73b0f24a288b43bfd29 Mon Sep 17 00:00:00 2001
From: Branislav Brzak <branislav.brzak@syrmia.com>
Date: Tue, 20 Jun 2023 16:19:55 +0200
Subject: [PATCH] * riscv-tdep.c (riscv_insn::fetch_instruction): Always fetch 
 instructions as LE

---
 gdb/riscv-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 500279e1ae9..ae18eb64452 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1812,7 +1812,6 @@ ULONGEST
 riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
   gdb_byte buf[RISCV_MAX_INSN_LEN];
   int instlen, status;
 
@@ -1833,7 +1832,8 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 	memory_error (TARGET_XFER_E_IO, addr + 2);
     }
 
-  return extract_unsigned_integer (buf, instlen, byte_order);
+  /* RISC-V Specification states instructions are always little endian */
+  return extract_unsigned_integer (buf, instlen, BFD_ENDIAN_LITTLE);
 }
 
 /* Fetch from target memory an instruction at PC and decode it.  This can
-- 
2.34.1

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Tom Tromey]

>>>>> Branislav Brzak <Branislav.Brzak@Syrmia.com> writes:

> Here is the patch with the changes you specified.

Looks great.

Approved-By: Tom Tromey <tom@tromey.com>

If you need me to commit it for you, let me know.

Tom

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Branislav Brzak]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

Hi Tom,

I'd like to do the commit myself, although I'm having some issues.
Namely, I've been following this guide https://sourceware.org/gdb/wiki/PushingToReleaseBranch
and I have the bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=30571
And locally I have the commit in the specified format.

The only thing I'm having an issue with is that I don't know how to proceed with this, as pusing
to master understandably gives me a 403, and I haven't seen any mention of pushing a patch
via a PR.

What would be the next step here?

Thanks,
Branislav

________________________________
From: Tom Tromey <tom@tromey.com>
Sent: Tuesday, June 20, 2023 6:42 PM
To: Branislav Brzak <Branislav.Brzak@Syrmia.com>
Cc: Tom Tromey <tom@tromey.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>; Dragoslav Sicarov <Dragoslav.Sicarov@Syrmia.com>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>
Subject: Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

>>>>> Branislav Brzak <Branislav.Brzak@Syrmia.com> writes:

> Here is the patch with the changes you specified.

Looks great.

Approved-By: Tom Tromey <tom@tromey.com>

If you need me to commit it for you, let me know.

Tom

Re: [PATCH 0/1] riscv: Ensure LE instruction fetching

[Tom Tromey]

> I'd like to do the commit myself, although I'm having some issues.
> Namely, I've been following this guide https://sourceware.org/gdb/wiki/PushingToReleaseBranch
> and I have the bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=30571
> And locally I have the commit in the specified format.

I'm checking this in for you.

The commit message on the actual patch was just a changelog entry.  gdb
doesn't use changelog any more, and it's better to write a descriptive
subject and text, so I took these from the "PATCH 0/0" email you sent.

Normally what I do for single patches is just use the commit message
directly as the email.  This helps ensure that sufficient information is
in there.  Then I "git send-email" instead of attaching it.

Tom