gdb: Remove a non-const reference parameter

  Non-const reference parameter should be avoided according to the GDB
coding standard:

  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead

This commit updates the gdbarch method gdbarch_stap_adjust_register,
and the one implementation i386_stap_adjust_register to avoid using a
non-const reference parameter.

I've also removed the kfail from the testsuite for bug 24541, as this
issue is now resolved.

gdb/ChangeLog:

	PR breakpoints/24541
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh: Adjust return type and parameter types for
	'stap_adjust_register'.
	(i386_stap_adjust_register): Adjust signature and return new
	register name.
	* stap-probe.c (stap_parse_register_operand): Adjust use of
	'gdbarch_stap_adjust_register'.

gdb/testsuite/ChangeLog:

	PR breakpoints/24541
	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to 24541.
---
 gdb/ChangeLog                                    | 12 ++++++++++++
 gdb/gdbarch.c                                    |  6 +++---
 gdb/gdbarch.h                                    |  4 ++--
 gdb/gdbarch.sh                                   |  2 +-
 gdb/i386-tdep.c                                  | 12 ++++++++----
 gdb/stap-probe.c                                 | 15 ++++++++-------
 gdb/testsuite/ChangeLog                          |  5 +++++
 gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
 8 files changed, 40 insertions(+), 38 deletions(-)
  

On 2019-07-02 11:21 a.m., Andrew Burgess wrote:
> Non-const reference parameter should be avoided according to the GDB
> coding standard:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
> 
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
> 
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.

Hi Andrew,

So I was the one who added this rule to the guide.  When I asked for opinions and
didn't get any, I thought it was because it wasn't very controversial and chose to add it.

Since then, Pedro mentioned a few times he didn't agree with it.

I'd like to know what others think, should we keep it (and enforce it) or remove it?

Simon
  

On Tuesday, July 02 2019, Andrew Burgess wrote:

> Non-const reference parameter should be avoided according to the GDB
> coding standard:
>
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
>
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
>
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.

Thanks, Andrew (and sorry for the delay; yesterday was a holiday here).

This is OK.

> gdb/ChangeLog:
>
> 	PR breakpoints/24541
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Adjust return type and parameter types for
> 	'stap_adjust_register'.
> 	(i386_stap_adjust_register): Adjust signature and return new
> 	register name.
> 	* stap-probe.c (stap_parse_register_operand): Adjust use of
> 	'gdbarch_stap_adjust_register'.
>
> gdb/testsuite/ChangeLog:
>
> 	PR breakpoints/24541
> 	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to 24541.
> ---
>  gdb/ChangeLog                                    | 12 ++++++++++++
>  gdb/gdbarch.c                                    |  6 +++---
>  gdb/gdbarch.h                                    |  4 ++--
>  gdb/gdbarch.sh                                   |  2 +-
>  gdb/i386-tdep.c                                  | 12 ++++++++----
>  gdb/stap-probe.c                                 | 15 ++++++++-------
>  gdb/testsuite/ChangeLog                          |  5 +++++
>  gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
>  8 files changed, 40 insertions(+), 38 deletions(-)
>
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index cc7d0ace66c..725b98fcd9f 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4530,14 +4530,14 @@ gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
>    return gdbarch->stap_adjust_register != NULL;
>  }
>  
> -void
> -gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
> +std::string
> +gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->stap_adjust_register != NULL);
>    if (gdbarch_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "gdbarch_stap_adjust_register called\n");
> -  gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
> +  return gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
>  }
>  
>  void
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 0857d2f3027..c3556d38419 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1376,8 +1376,8 @@ extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbar
>  
>  extern int gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch);
>  
> -typedef void (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> -extern void gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> +typedef std::string (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
> +extern std::string gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
>  extern void set_gdbarch_stap_adjust_register (struct gdbarch *gdbarch, gdbarch_stap_adjust_register_ftype *stap_adjust_register);
>  
>  /* DTrace related functions.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index f3d1bf489ac..2f9fbbc56cd 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1053,7 +1053,7 @@ M;int;stap_parse_special_token;struct stap_parse_info *p;p
>  # replace the register name from %ax to %eax.
>  #
>  # The rationale for this can be found at PR breakpoints/24541.
> -M;void;stap_adjust_register;struct stap_parse_info *p, std::string \&regname, int regnum;p, regname, regnum
> +M;std::string;stap_adjust_register;struct stap_parse_info *p, const std::string \&regname, int regnum;p, regname, regnum
>  
>  # DTrace related functions.
>  
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 00c1f8d7499..b956604178f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -4389,9 +4389,9 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  /* Implementation of 'gdbarch_stap_adjust_register', as defined in
>     gdbarch.h.  */
>  
> -static void
> +static std::string
>  i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
> -			   std::string &regname, int regnum)
> +			   const std::string &regname, int regnum)
>  {
>    static const std::unordered_set<std::string> reg_assoc
>      = { "ax", "bx", "cx", "dx",
> @@ -4402,14 +4402,18 @@ i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
>        /* If we're dealing with a register whose size is greater or
>  	 equal than the size specified by the "[-]N@" prefix, then we
>  	 don't need to do anything.  */
> -      return;
> +      return regname;
>      }
>  
>    if (reg_assoc.find (regname) != reg_assoc.end ())
>      {
>        /* Use the extended version of the register.  */
> -      regname = "e" + regname;
> +      std::string tmp = "e" + regname;
> +      return tmp;
>      }
> +
> +  /* Use the requested register.  */
> +  return regname;
>  }
>  
>  
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index aa1c8144d8a..747bbcfb8c1 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -774,23 +774,24 @@ stap_parse_register_operand (struct stap_parse_info *p)
>       code would like to perform on the register name.  */
>    if (gdbarch_stap_adjust_register_p (gdbarch))
>      {
> -      std::string oldregname = regname;
> +      std::string newregname
> +	= gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
>  
> -      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
> -
> -      if (regname != oldregname)
> +      if (regname != newregname)
>  	{
>  	  /* This is just a check we perform to make sure that the
>  	     arch-dependent code has provided us with a valid
>  	     register name.  */
> -	  regnum = user_reg_map_name_to_regnum (gdbarch, regname.c_str (),
> -						regname.size ());
> +	  regnum = user_reg_map_name_to_regnum (gdbarch, newregname.c_str (),
> +						newregname.size ());
>  
>  	  if (regnum == -1)
>  	    internal_error (__FILE__, __LINE__,
>  			    _("Invalid register name '%s' after replacing it"
>  			      " (previous name was '%s')"),
> -			    regname.c_str (), oldregname.c_str ());
> +			    newregname.c_str (), regname.c_str ());
> +
> +	  regname = newregname;
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> index fa5b11e3e58..4b87e4b62ba 100644
> --- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> +++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> @@ -130,27 +130,7 @@ with_test_prefix "all with invalid regexp" {
>      setup_catchpoint "throw" "-r blahblah"
>      setup_catchpoint "rethrow" "-r woofwoof"
>      setup_catchpoint "catch" "-r miowmiow"
> -
> -    # Would like to use 'continue_to_breakpoint_in_main' here, if
> -    # there wasn't a bug that requires a use of kfail.
> -
> -    mi_send_resuming_command "exec-continue" \
> -	"exec-continue"
> -    set testname "run until breakpoint in main"
> -    gdb_expect {
> -	-re "could not find minimal symbol for typeinfo address.*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,bkptno=\"$decimal\",reason=\"breakpoint-hit\",disp=\"keep\".*func=\"__cxa_throw\".*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,reason=\"breakpoint-hit\".*func=\"main\".*line=\"${main_lineno}\".*$mi_gdb_prompt$" {
> -	    pass "${testname}"
> -	}
> -	timeout {
> -	    fail "${testname} (timeout)"
> -	}
> -    }
> +    continue_to_breakpoint_in_main
>  }
>  
>  # Now check that all of the commands with a regexp that does match,
> -- 
> 2.14.5
  

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Non-const reference parameter should be avoided according to the GDB
Andrew> coding standard:

Andrew>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead

Andrew> This commit updates the gdbarch method gdbarch_stap_adjust_register,
Andrew> and the one implementation i386_stap_adjust_register to avoid using a
Andrew> non-const reference parameter.

Thanks for doing this.  I probably should have pushed back on the
original patch a little for this reason.

Andrew> +      std::string tmp = "e" + regname;
Andrew> +      return tmp;

This can just be `return "e" + regname' I think.

Looks ok otherwise.

Tom
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> So I was the one who added this rule to the guide.  When I asked for opinions and
Simon> didn't get any, I thought it was because it wasn't very controversial and chose to add it.

Simon> Since then, Pedro mentioned a few times he didn't agree with it.

Simon> I'd like to know what others think, should we keep it (and enforce it) or remove it?

Pedro raised good points on this topic, but in this particular case I
think the code is clearer without modifying the parameter.

Tom
  

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-07-02 16:21:19 +0100]:

> Non-const reference parameter should be avoided according to the GDB
> coding standard:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
> 
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
> 
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.
> 
> gdb/ChangeLog:
> 
> 	PR breakpoints/24541
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Adjust return type and parameter types for
> 	'stap_adjust_register'.
> 	(i386_stap_adjust_register): Adjust signature and return new
> 	register name.
> 	* stap-probe.c (stap_parse_register_operand): Adjust use of
> 	'gdbarch_stap_adjust_register'.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR breakpoints/24541
> 	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to
> 	24541.

I went ahead and pushed this patch, after making the improvement Tom
suggested.

There didn't seem to be a rush of voices suggesting the non-const rule
be dropped.  We can always revert this if the rule is later removed.

Thanks,
Andrew



> ---
>  gdb/ChangeLog                                    | 12 ++++++++++++
>  gdb/gdbarch.c                                    |  6 +++---
>  gdb/gdbarch.h                                    |  4 ++--
>  gdb/gdbarch.sh                                   |  2 +-
>  gdb/i386-tdep.c                                  | 12 ++++++++----
>  gdb/stap-probe.c                                 | 15 ++++++++-------
>  gdb/testsuite/ChangeLog                          |  5 +++++
>  gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
>  8 files changed, 40 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index cc7d0ace66c..725b98fcd9f 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4530,14 +4530,14 @@ gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
>    return gdbarch->stap_adjust_register != NULL;
>  }
>  
> -void
> -gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
> +std::string
> +gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->stap_adjust_register != NULL);
>    if (gdbarch_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "gdbarch_stap_adjust_register called\n");
> -  gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
> +  return gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
>  }
>  
>  void
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 0857d2f3027..c3556d38419 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1376,8 +1376,8 @@ extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbar
>  
>  extern int gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch);
>  
> -typedef void (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> -extern void gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> +typedef std::string (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
> +extern std::string gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
>  extern void set_gdbarch_stap_adjust_register (struct gdbarch *gdbarch, gdbarch_stap_adjust_register_ftype *stap_adjust_register);
>  
>  /* DTrace related functions.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index f3d1bf489ac..2f9fbbc56cd 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1053,7 +1053,7 @@ M;int;stap_parse_special_token;struct stap_parse_info *p;p
>  # replace the register name from %ax to %eax.
>  #
>  # The rationale for this can be found at PR breakpoints/24541.
> -M;void;stap_adjust_register;struct stap_parse_info *p, std::string \&regname, int regnum;p, regname, regnum
> +M;std::string;stap_adjust_register;struct stap_parse_info *p, const std::string \&regname, int regnum;p, regname, regnum
>  
>  # DTrace related functions.
>  
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 00c1f8d7499..b956604178f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -4389,9 +4389,9 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  /* Implementation of 'gdbarch_stap_adjust_register', as defined in
>     gdbarch.h.  */
>  
> -static void
> +static std::string
>  i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
> -			   std::string &regname, int regnum)
> +			   const std::string &regname, int regnum)
>  {
>    static const std::unordered_set<std::string> reg_assoc
>      = { "ax", "bx", "cx", "dx",
> @@ -4402,14 +4402,18 @@ i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
>        /* If we're dealing with a register whose size is greater or
>  	 equal than the size specified by the "[-]N@" prefix, then we
>  	 don't need to do anything.  */
> -      return;
> +      return regname;
>      }
>  
>    if (reg_assoc.find (regname) != reg_assoc.end ())
>      {
>        /* Use the extended version of the register.  */
> -      regname = "e" + regname;
> +      std::string tmp = "e" + regname;
> +      return tmp;
>      }
> +
> +  /* Use the requested register.  */
> +  return regname;
>  }
>  
>  
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index aa1c8144d8a..747bbcfb8c1 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -774,23 +774,24 @@ stap_parse_register_operand (struct stap_parse_info *p)
>       code would like to perform on the register name.  */
>    if (gdbarch_stap_adjust_register_p (gdbarch))
>      {
> -      std::string oldregname = regname;
> +      std::string newregname
> +	= gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
>  
> -      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
> -
> -      if (regname != oldregname)
> +      if (regname != newregname)
>  	{
>  	  /* This is just a check we perform to make sure that the
>  	     arch-dependent code has provided us with a valid
>  	     register name.  */
> -	  regnum = user_reg_map_name_to_regnum (gdbarch, regname.c_str (),
> -						regname.size ());
> +	  regnum = user_reg_map_name_to_regnum (gdbarch, newregname.c_str (),
> +						newregname.size ());
>  
>  	  if (regnum == -1)
>  	    internal_error (__FILE__, __LINE__,
>  			    _("Invalid register name '%s' after replacing it"
>  			      " (previous name was '%s')"),
> -			    regname.c_str (), oldregname.c_str ());
> +			    newregname.c_str (), regname.c_str ());
> +
> +	  regname = newregname;
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> index fa5b11e3e58..4b87e4b62ba 100644
> --- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> +++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> @@ -130,27 +130,7 @@ with_test_prefix "all with invalid regexp" {
>      setup_catchpoint "throw" "-r blahblah"
>      setup_catchpoint "rethrow" "-r woofwoof"
>      setup_catchpoint "catch" "-r miowmiow"
> -
> -    # Would like to use 'continue_to_breakpoint_in_main' here, if
> -    # there wasn't a bug that requires a use of kfail.
> -
> -    mi_send_resuming_command "exec-continue" \
> -	"exec-continue"
> -    set testname "run until breakpoint in main"
> -    gdb_expect {
> -	-re "could not find minimal symbol for typeinfo address.*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,bkptno=\"$decimal\",reason=\"breakpoint-hit\",disp=\"keep\".*func=\"__cxa_throw\".*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,reason=\"breakpoint-hit\".*func=\"main\".*line=\"${main_lineno}\".*$mi_gdb_prompt$" {
> -	    pass "${testname}"
> -	}
> -	timeout {
> -	    fail "${testname} (timeout)"
> -	}
> -    }
> +    continue_to_breakpoint_in_main
>  }
>  
>  # Now check that all of the commands with a regexp that does match,
> -- 
> 2.14.5
>