This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdb: Fix riscv ARI issues
On Friday, March 09 2018, Andrew Burgess wrote:
> 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.
Hi Andrew,
> * 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?
Oh, I guess I stand corrected, then.
> I'll fix the internationalisation as requested.
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/