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

Message ID 20200207150003.8383-4-shahab.vahedi@gmail.com
State New
Headers show
Series
  • Fixes for GDB Testsuites
Related show

Commit Message

Shahab Vahedi Feb. 7, 2020, 2:59 p.m.
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(-)

-- 
2.25.0

Comments

Luis Machado Feb. 11, 2020, 7:35 a.m. | #1
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.
Andrew Burgess Feb. 26, 2020, 7:45 p.m. | #2
* 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

>

Patch

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
     }