Re: [PATCH] gdb: Fix riscv ARI issues

[Sergio Durigan Junior <sergiodj@redhat.com>]

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/