[03/14] gdb/testsuite: Add exit_is_reliable proc

  From: Anton Kolesov <Anton.Kolesov@synopsys.com>

Add a function that returns 1 if exit is reliable or 0 otherwise. Previously
value of "board_info gdb,exit_is_reliable" was used only in one place, in
"gdb_continue_to_end", and sensible default value was evaluated at the same
location. However there are more places that need to check value of
"gdb,exit_is_reliable", so it makes sense to extract it into a separate
function that returns a reliable value.

gdb/testsuite/ChangeLog:
2016-07-13  Anton Kolesov <Anton.Kolesov@synopsys.com>

        * lib/gdb.exp (exit_is_reliable): Procedure is added.
        (gdb_continue_to_end): Refactor to use "exit_is_reliable".

Signed-off-by: Anton Kolesov <Anton.Kolesov@synopsys.com>
---
 gdb/testsuite/lib/gdb.exp | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
  

On 2/7/20 11:59 AM, Shahab Vahedi wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> Add a function that returns 1 if exit is reliable or 0 otherwise. Previously
> value of "board_info gdb,exit_is_reliable" was used only in one place, in
> "gdb_continue_to_end", and sensible default value was evaluated at the same
> location. However there are more places that need to check value of
> "gdb,exit_is_reliable", so it makes sense to extract it into a separate
> function that returns a reliable value.
> 
> gdb/testsuite/ChangeLog:
> 2016-07-13  Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
>          * lib/gdb.exp (exit_is_reliable): Procedure is added.
>          (gdb_continue_to_end): Refactor to use "exit_is_reliable".
> 
> Signed-off-by: Anton Kolesov <Anton.Kolesov@synopsys.com>
> ---
>   gdb/testsuite/lib/gdb.exp | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index eb1d145f2bb7..4376e08ca1b2 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3357,6 +3357,16 @@ proc use_gdb_stub {} {
>     return [target_info exists use_gdb_stub]
>   }
>   
> +# Return 1 if target has a reliable exit() function, 0 - otherwise.
> +
> +proc exit_is_reliable {} {
> +    if { [target_info exists exit_is_reliable] } {
> +      return [target_info exit_is_reliable]
> +    } else {
> +      return ![use_gdb_stub]
> +    }
> +}
> +
>   # Return 1 if the current remote target is an instance of our GDBserver, 0
>   # otherwise.  Return -1 if there was an error and we can't tell.
>   
> @@ -5287,13 +5297,7 @@ proc gdb_continue_to_end {{mssg ""} {command continue} {allow_extra 0}} {
>     # loop, or a forced crash/reset.  For native targets, by default, we
>     # assume process exit is reported as such.  If a non-reliable target
>     # is used, we set a breakpoint at exit, and continue to that.
> -  if { [target_info exists exit_is_reliable] } {
> -      set exit_is_reliable [target_info exit_is_reliable]
> -  } else {
> -      set exit_is_reliable [expr ! $use_gdb_stub]
> -  }
> -
> -  if { ! $exit_is_reliable } {
> +  if ![exit_is_reliable] {
>       if {![gdb_breakpoint "exit"]} {
>         return 0
>       }
> 

This LGTM as a refactor.
  

* Shahab Vahedi <shahab.vahedi@gmail.com> [2020-02-07 15:59:52 +0100]:

> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> Add a function that returns 1 if exit is reliable or 0 otherwise. Previously
> value of "board_info gdb,exit_is_reliable" was used only in one place, in
> "gdb_continue_to_end", and sensible default value was evaluated at the same
> location. However there are more places that need to check value of
> "gdb,exit_is_reliable", so it makes sense to extract it into a separate
> function that returns a reliable value.
> 
> gdb/testsuite/ChangeLog:
> 2016-07-13  Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
>         * lib/gdb.exp (exit_is_reliable): Procedure is added.
>         (gdb_continue_to_end): Refactor to use "exit_is_reliable".

This is fine in theory, but...

> 
> Signed-off-by: Anton Kolesov <Anton.Kolesov@synopsys.com>
> ---
>  gdb/testsuite/lib/gdb.exp | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index eb1d145f2bb7..4376e08ca1b2 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3357,6 +3357,16 @@ proc use_gdb_stub {} {
>    return [target_info exists use_gdb_stub]
>  }
>  
> +# Return 1 if target has a reliable exit() function, 0 - otherwise.
> +
> +proc exit_is_reliable {} {
> +    if { [target_info exists exit_is_reliable] } {
> +      return [target_info exit_is_reliable]
> +    } else {
> +      return ![use_gdb_stub]
> +    }
> +}

I don't see how the header comment on this proc matches that
implementation.  Maybe a comment more like this:

  Return non-zero if target has a reliable exit() function or if the
  target is not using the gdb stub (see USE_GDB_STUB).  Otherwise
  return false.

But otherwise seems like a reasonable clean up.

Thanks,
Andrew


> +
>  # Return 1 if the current remote target is an instance of our GDBserver, 0
>  # otherwise.  Return -1 if there was an error and we can't tell.
>  
> @@ -5287,13 +5297,7 @@ proc gdb_continue_to_end {{mssg ""} {command continue} {allow_extra 0}} {
>    # loop, or a forced crash/reset.  For native targets, by default, we
>    # assume process exit is reported as such.  If a non-reliable target
>    # is used, we set a breakpoint at exit, and continue to that.
> -  if { [target_info exists exit_is_reliable] } {
> -      set exit_is_reliable [target_info exit_is_reliable]
> -  } else {
> -      set exit_is_reliable [expr ! $use_gdb_stub]
> -  }
> -
> -  if { ! $exit_is_reliable } {
> +  if ![exit_is_reliable] {
>      if {![gdb_breakpoint "exit"]} {
>        return 0
>      }
> -- 
> 2.25.0
>