* [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
@ 2020-01-24 2:09 Jim Wilson
2020-01-24 13:32 ` Maciej W. Rozycki
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jim Wilson @ 2020-01-24 2:09 UTC (permalink / raw)
To: binutils; +Cc: macro, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-24 2:09 [PATCH] RISC-V: Fix gdbserver problem with handling arch strings Jim Wilson
@ 2020-01-24 13:32 ` Maciej W. Rozycki
2020-01-24 22:38 ` Jim Wilson
2020-01-27 23:24 ` [PATCH v2] " Jim Wilson
2020-01-30 15:41 ` [PATCH] " Palmer Dabbelt via binutils
2 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-01-24 13:32 UTC (permalink / raw)
To: Jim Wilson; +Cc: binutils
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-24 13:32 ` Maciej W. Rozycki
@ 2020-01-24 22:38 ` Jim Wilson
2020-01-27 13:04 ` Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Jim Wilson @ 2020-01-24 22:38 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Binutils
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-24 22:38 ` Jim Wilson
@ 2020-01-27 13:04 ` Maciej W. Rozycki
2020-01-27 20:31 ` Jim Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2020-01-27 13:04 UTC (permalink / raw)
To: Jim Wilson; +Cc: Binutils, gdb
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-27 13:04 ` Maciej W. Rozycki
@ 2020-01-27 20:31 ` Jim Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Jim Wilson @ 2020-01-27 20:31 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Binutils, gdb
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-24 2:09 [PATCH] RISC-V: Fix gdbserver problem with handling arch strings Jim Wilson
2020-01-24 13:32 ` Maciej W. Rozycki
@ 2020-01-27 23:24 ` Jim Wilson
2020-01-30 15:41 ` [PATCH] " Palmer Dabbelt via binutils
2 siblings, 0 replies; 7+ messages in thread
From: Jim Wilson @ 2020-01-27 23:24 UTC (permalink / raw)
To: binutils; +Cc: 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Fix gdbserver problem with handling arch strings.
2020-01-24 2:09 [PATCH] RISC-V: Fix gdbserver problem with handling arch strings Jim Wilson
2020-01-24 13:32 ` Maciej W. Rozycki
2020-01-27 23:24 ` [PATCH v2] " Jim Wilson
@ 2020-01-30 15:41 ` Palmer Dabbelt via binutils
2 siblings, 0 replies; 7+ messages in thread
From: Palmer Dabbelt via binutils @ 2020-01-30 15:41 UTC (permalink / raw)
To: Jim Wilson, macro; +Cc: Jim Wilson, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-30 15:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 2:09 [PATCH] RISC-V: Fix gdbserver problem with handling arch strings Jim Wilson
2020-01-24 13:32 ` Maciej W. Rozycki
2020-01-24 22:38 ` Jim Wilson
2020-01-27 13:04 ` Maciej W. Rozycki
2020-01-27 20:31 ` Jim Wilson
2020-01-27 23:24 ` [PATCH v2] " Jim Wilson
2020-01-30 15:41 ` [PATCH] " Palmer Dabbelt via binutils
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).