gdb: Fix riscv ARI issues

Message ID 20180307224121.19941-1-andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb: Fix riscv ARI issues
Related show

Commit Message

Andrew Burgess March 7, 2018, 10:41 p.m.
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(-)

-- 
2.14.3

Comments

Sergio Durigan Junior March 8, 2018, 5:38 p.m. | #1
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/
Simon Marchi March 9, 2018, 4:04 a.m. | #2
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
Andrew Burgess March 9, 2018, 10:43 a.m. | #3
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/
Sergio Durigan Junior March 9, 2018, 4:32 p.m. | #4
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/

Patch

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)