[PATCH] gdb: Fix riscv ARI issues
Andrew Burgess
andrew.burgess@embecosm.com
Fri Mar 9 10:43:00 GMT 2018
Sergio,
Thanks for taking the time to review my patch. I just wanted to
follow up on your feedback to help clarify the coding standard.
* Sergio Durigan Junior <sergiodj@redhat.com> [2018-03-08 12:38:06 -0500]:
> On Wednesday, March 07 2018, Andrew Burgess wrote:
>
> > Fixes some ARI issues in recently added riscv code, the ARI email is:
> >
> > https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html
>
> Thanks for the patch, Andrew. Comment below.
>
> > gdb/ChangeLog:
> >
> > * riscv-tdep.c (riscv_register_name): Use xsnprintf instead of
> > sprintf.
> > (riscv_insn::fetch_instruction): Use gdb_assert instead of
> > internal_error.
> > (riscv_print_arg_location): Use gdb_assert_not_reached instead of
> > error.
> > (riscv_push_dummy_call): Likewise.
> > ---
> > gdb/ChangeLog | 10 ++++++++++
> > gdb/riscv-tdep.c | 14 ++++++--------
> > 2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> > index 11b12279321..d84e7aba76a 100644
> > --- a/gdb/riscv-tdep.c
> > +++ b/gdb/riscv-tdep.c
> > @@ -481,7 +481,7 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
> > {
> > static char buf[20];
> >
> > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM);
> > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM);
> > return buf;
> > }
> >
> > @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
> >
> > /* If we need more, grab it now. */
> > instlen = riscv_insn_length (buf[0]);
> > + gdb_assert (instlen <= sizeof (buf));
> > *len = instlen;
> > - if (instlen > sizeof (buf))
> > - internal_error (__FILE__, __LINE__,
> > - _("%s: riscv_insn_length returned %i"),
> > - __func__, instlen);
> > - else if (instlen > 2)
> > +
> > + if (instlen > 2)
> > {
> > status = target_read_memory (addr + 2, buf + 2, instlen - 2);
> > if (status)
> > @@ -2009,7 +2007,7 @@ riscv_print_arg_location (ui_file *stream, struct gdbarch *gdbarch,
> > break;
> >
> > default:
> > - error ("unknown argument location type");
> > + gdb_assert_not_reached ("unknown argument location type");
>
> Strings must be marked for localization, with _():
>
> gdb_assert_not_reached (_("unknown argument location type"));
>
> I also think it's a good thing to start the sentence with a capital
> letter.
I referenced the GNU Standard here:
https://www.gnu.org/prep/standards/standards.html#Errors
Specifically these two paragraphs are I think relevant:
"The string message should not begin with a capital letter when it
follows a program name and/or file name, because that isn’t the
beginning of a sentence. (The sentence conceptually starts at the
beginning of the line.) Also, it should not end with a period.
Error messages from interactive programs, and other messages such
as usage messages, should start with a capital letter. But they
should not end with a period."
Though GDB is clearly an interactive program and so the second
paragraph should apply, the 'gdb_assert_not_reached' does prefix the
message with some text like this:
filename:line: internal-error: function: message
So, to me, it feels like the first paragraph should apply and the
message should start with no capital letter.
Further, a survey of the 157 uses of gdb_assert_not_reached show:
142 start with a lower case letter
1 starts '...'
7 start with a capital letter
6 start with LOC_COMPUTED
1 is the empty string
So, should message strings that are going to be prefixed be
capitalised?
I'll fix the internationalisation as requested.
Thanks,
Andrew
>
> > }
> > }
> >
> > @@ -2149,7 +2147,7 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
> > break;
> >
> > default:
> > - error ("unknown argument location type");
> > + gdb_assert_not_reached ("unknown argument location type");
>
> Likewise.
>
> > }
> >
> > if (second_arg_length > 0)
> > --
> > 2.14.3
>
> Thanks,
>
> --
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
More information about the Gdb-patches
mailing list