[PATCH] gdb: Fix riscv ARI issues

[PATCH] gdb: Fix riscv ARI issues

[Andrew Burgess]

Fixes some ARI issues in recently added riscv code, the ARI email is:

  https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html

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");
     }
 }
 
@@ -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");
 	}
 
       if (second_arg_length > 0)
-- 
2.14.3

Re: [PATCH] gdb: Fix riscv ARI issues

[Sergio Durigan Junior]

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.

>      }
>  }
>  
> @@ -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/

Re: [PATCH] gdb: Fix riscv ARI issues

[Andrew Burgess]

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/

Re: [PATCH] gdb: Fix riscv ARI issues

[Sergio Durigan Junior]

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/

Re: [PATCH] gdb: Fix riscv ARI issues

[Simon Marchi]

Hi Andrew,

On top of what Sergio said:

> 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);

sizeof (buf) instead of hard-coding 20?

Thanks,

Simon