Re: [PATCH] gdb: Fix riscv ARI issues

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

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/