[PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Jim Wilson]

Maciej reported a problem found by his RISC-V gdbserver port.
warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
warning: Could not load XML target description; ignoring

We only have two arches defined, riscv:rv32 and riscv:rv64.  Both bfd and
gdb are creating arch strings that have extension letters added to the base
architecture.  The bfd_default_scan function requires an exact match, so
these strings fail to map to a bfd_arch.  I think we should ignore the
extension letters in a RISC-V specific scan function.

Tested with riscv{32,64}-{elf,linux} cross build and test with no regressions.

Not committed yet in case anyone wanted to comment on it before I check it in.

Jim

	bfd/
	* cpu-riscv.c (scan): New.
	(N): Change bfd_default_scan to scan.

Change-Id: I1498390cc4cc827a947eff039278f21c628e1fa3
---
 bfd/cpu-riscv.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/bfd/cpu-riscv.c b/bfd/cpu-riscv.c
index bc90ffc876..bfd98d4e95 100644
--- a/bfd/cpu-riscv.c
+++ b/bfd/cpu-riscv.c
@@ -39,6 +39,23 @@ riscv_compatible (const bfd_arch_info_type *a, const bfd_arch_info_type *b)
   return a;
 }
 
+/* Return TRUE if STRING matches the architecture described by INFO.  */
+
+static bfd_boolean
+scan (const struct bfd_arch_info *info, const char *string)
+{
+  if (bfd_default_scan (info, string))
+    return TRUE;
+
+  /* The string might have extra characters for supported subsets.  So allow
+     a match that ignores trailing characters in string.  */
+  if (strncasecmp (string, info->printable_name,
+		   strlen (info->printable_name)) == 0)
+    return TRUE;
+
+  return FALSE;
+}
+
 #define N(BITS, NUMBER, PRINT, DEFAULT, NEXT)			\
   {								\
     BITS,      /* Bits in a word.  */				\
@@ -51,7 +68,7 @@ riscv_compatible (const bfd_arch_info_type *a, const bfd_arch_info_type *b)
     3,								\
     DEFAULT,							\
     riscv_compatible,						\
-    bfd_default_scan,						\
+    scan,							\
     bfd_arch_default_fill,					\
     NEXT,							\
     0 /* Maximum offset of a reloc from the start of an insn.  */\
-- 
2.17.1

Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Maciej W. Rozycki]

On Thu, 23 Jan 2020, Jim Wilson wrote:

> Maciej reported a problem found by his RISC-V gdbserver port.
> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
> warning: Could not load XML target description; ignoring
> 
> We only have two arches defined, riscv:rv32 and riscv:rv64.  Both bfd and
> gdb are creating arch strings that have extension letters added to the base
> architecture.  The bfd_default_scan function requires an exact match, so
> these strings fail to map to a bfd_arch.  I think we should ignore the
> extension letters in a RISC-V specific scan function.

 I think it's an acceptable solution short-term; after all it's not going 
to regress functionality.  However ultimately I think we ought to actually
interpret these suffix letters and arm the disassembler accordingly.

> Tested with riscv{32,64}-{elf,linux} cross build and test with no regressions.

 I'll push it through GDB testing with `gdbserver' yet, once my current 
native testing has completed (which BTW will take till the end of today 
only as it seems to run ~4 times faster now; presumably some test cases do 
not time out anymore).

> Not committed yet in case anyone wanted to comment on it before I check it in.

 I'd suggest naming the new function `riscv_scan' or suchlike, even though 
it's static, so as not to pollute the generic namespace.  We even have a 
precedent already with `riscv_compatible' nearby.

 Thanks for the quick fix!

  Maciej

Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Jim Wilson]

On Fri, Jan 24, 2020 at 5:32 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>  I think it's an acceptable solution short-term; after all it's not going
> to regress functionality.  However ultimately I think we ought to actually
> interpret these suffix letters and arm the disassembler accordingly.

The strings are checked when used as options, and when used in elf
attributes.  I'm not sure if they need to be checked here, but it is
probably a good idea to do that eventually.  Checking the strings for
correctness is complicated, as there are many different possible
correct answers, which is why I didn't try to do it in the first
version of the patch.  it would be nice to reuse some of the other
support, maybe the attribute merging support, but it would have to be
rewritten a bit to make this work.  I'd rather worry about that later,
or ask someone else to do it.

>  I'd suggest naming the new function `riscv_scan' or suchlike, even though
> it's static, so as not to pollute the generic namespace.  We even have a
> precedent already with `riscv_compatible' nearby.

arm and aarch64 are the only ports that define such a function, and
they both call it scan.  But I agree that riscv_scan is better.  I
changed it.

Jim

Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Maciej W. Rozycki]

Hi Jim,

 Cc-ing <gdb@sourceware.org> for wider audience.

> >  I think it's an acceptable solution short-term; after all it's not going
> > to regress functionality.  However ultimately I think we ought to actually
> > interpret these suffix letters and arm the disassembler accordingly.
> 
> The strings are checked when used as options, and when used in elf
> attributes.  I'm not sure if they need to be checked here, but it is
> probably a good idea to do that eventually.  Checking the strings for
> correctness is complicated, as there are many different possible
> correct answers, which is why I didn't try to do it in the first
> version of the patch.  it would be nice to reuse some of the other
> support, maybe the attribute merging support, but it would have to be
> rewritten a bit to make this work.  I'd rather worry about that later,
> or ask someone else to do it.

 Sure.

 I have test results now and they are looking good; there are several 
progressions from failures like:

Remote 'g' packet reply is too long (expected 264 bytes, got 532 bytes): 00000000000000002a6f61551500000040faffff3f0000000028010000000000b03857551500000060ad5f5515000000906e615515000000802401000000000050faffff3f00000000000000000000000100000000000000a8fbffff3f000000b8fbffff3f000000000000000000000078faffff3f0000006e05010000000000589c6f55150000004e4b024946434a2f180957551500000080e1baaa2a0000000000000000000000c000bbaa2a00000080e1baaa2a0000006804baaa2a000000000000000000000000000000000000007053baaa2a0000008252b2aa2a00000090fe01000000000048e056551500000004000000000000004000000000000000760501000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000xxxxxxxxxxxxxxxx00000000
(gdb) FAIL: gdb.base/break-unload-file.exp: cmdline: always-inserted off: break: file

which is obviously a sign of non-XML support heuristics failing.  There's 
some usual noise from gdb.threads/ tests too; not too much though.

 For the record overall results without and with your change are:

		=== gdb Summary ===

# of expected passes		59831
# of unexpected failures	767
# of unexpected successes	2
# of expected failures		47
# of unknown successes		5
# of known failures		101
# of unresolved testcases	9
# of untested testcases		122
# of unsupported tests		235

and:

		=== gdb Summary ===

# of expected passes		59858
# of unexpected failures	737
# of unexpected successes	2
# of expected failures		47
# of unknown successes		5
# of known failures		101
# of unresolved testcases	9
# of untested testcases		122
# of unsupported tests		235

respectively.  Also native results for a reference:

		=== gdb Summary ===

# of expected passes		57572
# of unexpected failures	1756
# of expected failures		58
# of unknown successes		3
# of known failures		85
# of unresolved testcases	118
# of untested testcases		161
# of unsupported tests		399

These are significantly worse as you can see (and don't cover gdb.ada/ 
tests, which all scored UNSUPPORTED status due to the lack of an Ada 
compiler), and my brief look has indicated that the additional failures 
are in many cases in tests that are not run in a remote setup, and in 
other cases they are genuine regressions such as (remote):

p/d check_arg_struct_01_01 (ref_val_struct_01_01)
$1 = 1
(gdb) PASS: gdb.base/infcall-nested-structs.exp: l=c++: types-tf: p/d check_arg_struct_01_01 (ref_val_struct_01_01)

vs (native):

p/d check_arg_struct_01_01 (ref_val_struct_01_01)
$1 = 0
(gdb) FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-tf: p/d check_arg_struct_01_01 (ref_val_struct_01_01)

which might be a compiler problem (old `gcc (GCC) 8.2.1 20181105 (Red Hat 
8.2.1-5)' in the native installation here, vs `riscv64-linux-gnu-gcc (GCC) 
10.0.0 20191109 (experimental)' in the remote one), which I'll look into 
getting sorted independently, perhaps by wiring a native compiler build 
into my toolchain compilation.

 So I think we're good to go.  I'll post updated `gdbserver' patches 
shortly.

 NB I think eventually we ought to get rid of the heuristics.  We are now 
well after Y2004, which is when XML target description support was added, 
and support for the RISC-V target has also been added well after then.  
So we should have enforced XML support in stubs since the beginning.

 Now mistakes happen and we didn't for one reason or another, but I think 
we ought to fix it.  If someone still insists on living in non-XML world 
with their debug stub, then they can use:

(gdb) set tdesc filename <whatever>

to get GDB set up for their long obsolete world.

 I won't rush implementing that requirement as I have other priorities 
now, but I mean to do this sometime unless someone beats me to it.  All 
the heuristics can go then.

  Maciej

Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Jim Wilson]

On Mon, Jan 27, 2020 at 5:04 AM Maciej W. Rozycki <macro@wdc.com> wrote:
> vs (native):
> p/d check_arg_struct_01_01 (ref_val_struct_01_01)
> $1 = 0
> (gdb) FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-tf: p/d check_arg_struct_01_01 (ref_val_struct_01_01)
>
> which might be a compiler problem (old `gcc (GCC) 8.2.1 20181105 (Red Hat
> 8.2.1-5)' in the native installation here, vs `riscv64-linux-gnu-gcc (GCC)
> 10.0.0 20191109 (experimental)' in the remote one), which I'll look into
> getting sorted independently, perhaps by wiring a native compiler build
> into my toolchain compilation.

This is
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89627
which was fixed by Andrew Burgess in gcc 9.

Jim

[PATCH v2] RISC-V: Fix gdbserver problem with handling arch strings.

[Jim Wilson]

No comments other than the one from Maciej, so I committed it with the change
he suggested.

Jim

	bfd/
	* cpu-riscv.c (riscv_scan): New.
	(N): Change bfd_default_scan to riscv_scan.

Change-Id: I66a6b7b7ed3d87d39b69dc327cc7b5f09b1deb20
---
 bfd/cpu-riscv.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/bfd/cpu-riscv.c b/bfd/cpu-riscv.c
index bc90ffc876..b5c972ff4d 100644
--- a/bfd/cpu-riscv.c
+++ b/bfd/cpu-riscv.c
@@ -39,6 +39,23 @@ riscv_compatible (const bfd_arch_info_type *a, const bfd_arch_info_type *b)
   return a;
 }
 
+/* Return TRUE if STRING matches the architecture described by INFO.  */
+
+static bfd_boolean
+riscv_scan (const struct bfd_arch_info *info, const char *string)
+{
+  if (bfd_default_scan (info, string))
+    return TRUE;
+
+  /* The string might have extra characters for supported subsets.  So allow
+     a match that ignores trailing characters in string.  */
+  if (strncasecmp (string, info->printable_name,
+		   strlen (info->printable_name)) == 0)
+    return TRUE;
+
+  return FALSE;
+}
+
 #define N(BITS, NUMBER, PRINT, DEFAULT, NEXT)			\
   {								\
     BITS,      /* Bits in a word.  */				\
@@ -51,7 +68,7 @@ riscv_compatible (const bfd_arch_info_type *a, const bfd_arch_info_type *b)
     3,								\
     DEFAULT,							\
     riscv_compatible,						\
-    bfd_default_scan,						\
+    riscv_scan,							\
     bfd_arch_default_fill,					\
     NEXT,							\
     0 /* Maximum offset of a reloc from the start of an insn.  */\
-- 
2.17.1

Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.

[Palmer Dabbelt via binutils]

On Fri, 24 Jan 2020 13:32:13 GMT (+0000), macro@wdc.com wrote:
> On Thu, 23 Jan 2020, Jim Wilson wrote:
>
>> Maciej reported a problem found by his RISC-V gdbserver port.
>> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
>> warning: Could not load XML target description; ignoring
>>
>> We only have two arches defined, riscv:rv32 and riscv:rv64.  Both bfd and
>> gdb are creating arch strings that have extension letters added to the base
>> architecture.  The bfd_default_scan function requires an exact match, so
>> these strings fail to map to a bfd_arch.  I think we should ignore the
>> extension letters in a RISC-V specific scan function.
>
>  I think it's an acceptable solution short-term; after all it's not going
> to regress functionality.  However ultimately I think we ought to actually
> interpret these suffix letters and arm the disassembler accordingly.
>
>> Tested with riscv{32,64}-{elf,linux} cross build and test with no regressions.
>
>  I'll push it through GDB testing with `gdbserver' yet, once my current
> native testing has completed (which BTW will take till the end of today
> only as it seems to run ~4 times faster now; presumably some test cases do
> not time out anymore).
>
>> Not committed yet in case anyone wanted to comment on it before I check it in.
>
>  I'd suggest naming the new function `riscv_scan' or suchlike, even though
> it's static, so as not to pollute the generic namespace.  We even have a
> precedent already with `riscv_compatible' nearby.
>
>  Thanks for the quick fix!

Yep, feel free to commit it -- I still don't have my SSH key set up over here
yet...

Thanks!

>
>   Maciej