[RFC] GDB testsuite patch. (revised version)

[RFC] GDB testsuite patch. (revised version)

[Manoj Iyer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1448 bytes --]


Michael,

Ok here is another attempt, let me know what you think of this one. I
added the change log to the begning of the attached file, and I am pasting
the same below.

2004-08-26  Manoj Iyer  <manjo@austin.ibm.com

        * lib/gdb.exp (gdb_check_debuginfo): New procedure
        * gdb.gdb/complaints.exp: check if gdb has debug information.
        * gdb.gdb/observer.exp: check if gdb has debug information.
        * gdb.gdb/selftest.exp: check if gdb has debug information.
        * gdb.gdb/xfullpath.exp: check if gdb has debug information.


Thanks
-----
Manjo
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ Cognito ergo sum                                                          +
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

On Wed, 25 Aug 2004, Michael Chastain wrote:

> Daniel Jacobowitz <drow@false.org> wrote:
> > That's not the same as what you've done.  gdb_file_cmd should not
> > always fail for objects without debugging information, since there are
> > other tests that work OK without it.  This only applies to the gdb.gdb/
> > tests.
>
> Damn, I noticed this, but I got so caught up in the procedural criticism,
> I forgot to mention it.
>
> Daniel is right; gdb_file_cmd is not a good place to test for this
> unconditionally.  You have to look in gdb.gdb/*.exp and find a good
> way so that you make this check only for gdb.gdb/*.exp.
>
> Michael
>

[-- Attachment #2: testsuite patch --]
[-- Type: TEXT/PLAIN, Size: 5585 bytes --]

2004-08-26  Manoj Iyer  <manjo@austin.ibm.com

        * lib/gdb.exp (gdb_check_debuginfo): New procedure
        * gdb.gdb/complaints.exp: check if gdb has debug information.
        * gdb.gdb/observer.exp: check if gdb has debug information.
        * gdb.gdb/selftest.exp: check if gdb has debug information.
        * gdb.gdb/xfullpath.exp: check if gdb has debug information.

=======================================================================

diff -Naur ./old/src/gdb/testsuite/gdb.gdb/complaints.exp ./new/src/gdb/testsuite/gdb.gdb/complaints.exp
--- ./old/src/gdb/testsuite/gdb.gdb/complaints.exp	2004-08-24 18:16:00.889191248 -0500
+++ ./new/src/gdb/testsuite/gdb.gdb/complaints.exp	2004-08-26 04:40:28.377294120 -0500
@@ -302,13 +302,20 @@
 
 gdb_start
 
+
 set file [remote_download host $GDB_FULLPATH x$tool]
 
+set debug_result [gdb_check_debuginfo x$tool ]
+if {$debug_result < 0} then {
+   return -1
+}
+
 set setup_result [setup_test $file ]
 if {$setup_result <0} then {
     return -1
 }
 
+
 test_initial_complaints
 test_serial_complaints
 test_short_complaints
diff -Naur ./old/src/gdb/testsuite/gdb.gdb/observer.exp ./new/src/gdb/testsuite/gdb.gdb/observer.exp
--- ./old/src/gdb/testsuite/gdb.gdb/observer.exp	2004-08-24 18:16:00.889191248 -0500
+++ ./new/src/gdb/testsuite/gdb.gdb/observer.exp	2004-08-26 04:40:41.855242136 -0500
@@ -254,6 +254,12 @@
 
 gdb_start
 set file [remote_download host $GDB_FULLPATH x$tool]
+
+set debug_result [gdb_check_debuginfo x$tool ]
+if {$debug_result < 0} then {
+   return -1
+}
+
 set result [test_observer_normal_stop $file];
 gdb_exit;
 catch "remote_file host delete $file";
diff -Naur ./old/src/gdb/testsuite/gdb.gdb/selftest.exp ./new/src/gdb/testsuite/gdb.gdb/selftest.exp
--- ./old/src/gdb/testsuite/gdb.gdb/selftest.exp	2004-08-24 18:16:00.889191248 -0500
+++ ./new/src/gdb/testsuite/gdb.gdb/selftest.exp	2004-08-26 04:40:57.197305976 -0500
@@ -507,6 +507,12 @@
 
 gdb_start
 set file [remote_download host $GDB_FULLPATH x$tool]
+
+set debug_result [gdb_check_debuginfo x$tool ]
+if {$debug_result < 0} then {
+   return -1
+}
+
 set result [test_with_self $file];
 gdb_exit;
 catch "remote_file host delete $file";
diff -Naur ./old/src/gdb/testsuite/gdb.gdb/xfullpath.exp ./new/src/gdb/testsuite/gdb.gdb/xfullpath.exp
--- ./old/src/gdb/testsuite/gdb.gdb/xfullpath.exp	2004-08-24 18:16:00.889191248 -0500
+++ ./new/src/gdb/testsuite/gdb.gdb/xfullpath.exp	2004-08-26 04:41:11.206306432 -0500
@@ -179,6 +179,12 @@
 
 gdb_start
 set file [remote_download host $GDB_FULLPATH x$tool]
+
+set debug_result [gdb_check_debuginfo x$tool ]
+if {$debug_result < 0} then {
+   return -1
+}
+
 set result [test_with_self $file];
 gdb_exit;
 catch "remote_file host delete $file";
diff -Naur ./old/src/gdb/testsuite/lib/gdb.exp ./new/src/gdb/testsuite/lib/gdb.exp
--- ./old/src/gdb/testsuite/lib/gdb.exp	2004-08-24 18:16:00.892190792 -0500
+++ ./new/src/gdb/testsuite/lib/gdb.exp	2004-08-26 04:40:01.645231192 -0500
@@ -1023,6 +1023,82 @@
     }
 }
 
+
+#
+# Check to see if the executable has debug information. 
+# If not the return -1 and exit.
+#
+proc gdb_check_debuginfo { arg } {
+    global verbose
+    global loadpath
+    global loadfile
+    global GDB
+    global gdb_prompt
+    upvar timeout timeout
+
+    if [is_remote host] {
+	set arg [remote_download host $arg];
+	if { $arg == "" } {
+	    error "download failed"
+	    return -1;
+	}
+    }
+
+    send_gdb "file $arg\n"
+    gdb_expect 120 {
+	-re "Reading symbols from.*no debugging symbols found.*$gdb_prompt $" {
+	    untested "skip tests no debugging information found."
+	    return -1
+	}
+        -re "Reading symbols from.*done.*$gdb_prompt $" {
+            verbose "\t\tLoaded $arg into the $GDB"
+            return 0
+        }
+        -re "has no symbol-table.*$gdb_prompt $" {
+            perror "$arg wasn't compiled with \"-g\""
+            return -1
+        }
+        -re "A program is being debugged already.*Kill it.*y or n. $" {
+            send_gdb "y\n"
+                verbose "\t\tKilling previous program being debugged"
+            exp_continue
+        }
+        -re "Load new symbol table from \".*\".*y or n. $" {
+            send_gdb "y\n"
+            gdb_expect 120 {
+                -re "Reading symbols from.*done.*$gdb_prompt $" {
+                    verbose "\t\tLoaded $arg with new symbol table into $GDB"
+                    return 0
+                }
+                timeout {
+                    perror "(timeout) Couldn't load $arg, other program already loaded."
+                    return -1
+                }
+            }
+	}
+        -re "No such file or directory.*$gdb_prompt $" {
+            perror "($arg) No such file or directory\n"
+            return -1
+        }
+        -re "$gdb_prompt $" {
+            perror "couldn't load $arg into $GDB."
+            return -1
+            }
+        timeout {
+            perror "couldn't load $arg into $GDB (timed out)."
+            return -1
+        }
+        eof {
+            # This is an attempt to detect a core dump, but seems not to
+            # work.  Perhaps we need to match .* followed by eof, in which
+            # gdb_expect does not seem to have a way to do that.
+            perror "couldn't load $arg into $GDB (end of file)."
+            return -1
+        }
+    }
+}
+
+
 #
 # start gdb -- start gdb running, default procedure
 #

Re: [RFC] GDB testsuite patch. (revised version)

[Michael Chastain]

Ouch, copying the whole text of gdb_file_cmd is no good, either!
That much duplicate code is bad.

Here is my plan.

gdb_file_cmd has a return value.  None of the callers of gdb_file_cmd
uses the return vale, except for gdb_load, which simply returns the same
value.

There are about 300 callers of gdb_load.  Most of them don't use the
return value either.  The exceptions are:

  gdb.base/remote.exp
  gdb.base/sepdebug.exp
    # does not use the return value but it wants to
  gdb.gdb/complaints.exp
  gdb.gdb/observer.exp
  gdb.gdb/selftest.exp
  gdb.gdb/xfullpath.exp
  gdb.stabs/weird.exp
    # does not use the return value but it wants to
  lib/gdb.exp:
    proc gdb_run_cmd

So, I will do some infrastructure work: I will make gdb_load return
a better return value that includes more information, like a list
with more status information:

  success indicator
  whatever sepdebug.exp is looking for
  whatever weird.exp is looking for
  indicator of whether debug symbols found or not

While I am in there, I will see if I can get back any commonality
with mi_gdb_file_cmd.  But that's optional.

I expect to commit the new gdb_file_cmd sometime on Saturday.  After
that, it will be easy for you to change gdb.gdb/*.exp to just use the
new information in the return value from gdb_load.

Michael

2004-08-26  Manoj Iyer  <manjo@austin.ibm.com

        * lib/gdb.exp (gdb_check_debuginfo): New procedure
        * gdb.gdb/complaints.exp: check if gdb has debug information.
        * gdb.gdb/observer.exp: check if gdb has debug information.
        * gdb.gdb/selftest.exp: check if gdb has debug information.
        * gdb.gdb/xfullpath.exp: check if gdb has debug information.

Re: [RFC] GDB testsuite patch. (revised version)

[Manoj Iyer]

The problem is that there are other tests that run just fine even if no
debugging symbols are found. This means if you change gdb_load to check if
no debugging symbols are found and return a -ive value this might harm the
other testcase.

Or

gdb_file_cmd can return a positive number like '1' when no debugging
information is found which can be checked for in gdb.gdb/*.exp and exit
the testcase. This way the other tests are not effected coz they check for
$return < 0

I will be happy to submit that patch, and see if you like my idea.

-----
manjo
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ Cognito ergo sum                                                          +
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

On Fri, 27 Aug 2004, Michael Chastain wrote:

> Ouch, copying the whole text of gdb_file_cmd is no good, either!
> That much duplicate code is bad.
>
> Here is my plan.
>
> gdb_file_cmd has a return value.  None of the callers of gdb_file_cmd
> uses the return vale, except for gdb_load, which simply returns the same
> value.
>
> There are about 300 callers of gdb_load.  Most of them don't use the
> return value either.  The exceptions are:
>
>   gdb.base/remote.exp
>   gdb.base/sepdebug.exp
>     # does not use the return value but it wants to
>   gdb.gdb/complaints.exp
>   gdb.gdb/observer.exp
>   gdb.gdb/selftest.exp
>   gdb.gdb/xfullpath.exp
>   gdb.stabs/weird.exp
>     # does not use the return value but it wants to
>   lib/gdb.exp:
>     proc gdb_run_cmd
>
> So, I will do some infrastructure work: I will make gdb_load return
> a better return value that includes more information, like a list
> with more status information:
>
>   success indicator
>   whatever sepdebug.exp is looking for
>   whatever weird.exp is looking for
>   indicator of whether debug symbols found or not
>
> While I am in there, I will see if I can get back any commonality
> with mi_gdb_file_cmd.  But that's optional.
>
> I expect to commit the new gdb_file_cmd sometime on Saturday.  After
> that, it will be easy for you to change gdb.gdb/*.exp to just use the
> new information in the return value from gdb_load.
>
> Michael
>
> 2004-08-26  Manoj Iyer  <manjo@austin.ibm.com
>
>         * lib/gdb.exp (gdb_check_debuginfo): New procedure
>         * gdb.gdb/complaints.exp: check if gdb has debug information.
>         * gdb.gdb/observer.exp: check if gdb has debug information.
>         * gdb.gdb/selftest.exp: check if gdb has debug information.
>         * gdb.gdb/xfullpath.exp: check if gdb has debug information.
>

Re: [RFC] GDB testsuite patch. (revised version)

[Michael Chastain]

Manoj Iyer <manjo@austin.ibm.com> wrote:
> The problem is that there are other tests that run just fine even if no
> debugging symbols are found. This means if you change gdb_load to check if
> no debugging symbols are found and return a -ive value this might harm the
> other testcase.

Right.  You tried that, and Daniel pointed out that it was wrong,
and I agreed.

> I will be happy to submit that patch, and see if you like my idea.

It's such a good idea, I already started implementing something
like that.

Here is the new header for gdb_file_cmd:

  # Load a file into the debugger.
  # The return value is a list with the following information:
  #
  #  { message word ... }
  #
  # MESSAGE has the following values:
  #
  #   ""     file was loaded successfully
  #   "..."  file was not loaded successfully.
  #          An error or perror has been generated with MESSAGE.
  #
  # If the MESSAGE is "", then there is an optional set of words.
  # The words may be:
  #
  #  nodebug  this file does not contain debug information
  #
  # TODO: gdb.base/sepdebug.exp and gdb.stabs/weird.exp might
  # be able to use this if they can get more information
  # in the return value.

TCL is all about lists and strings, and it makes life a lot
easier to return a nice list of strings.

If this still looks like a good idea in a few hours, and I commit it,
then you can write gdb.gdb/*.exp like this:

  set oldtimeout $timeout
  set timeout 600
  verbose "Timeout is now $timeout seconds" 2

  set status [gdb_load $executable]

  set timeout $oldtimeout
  verbose "Timeout is now $timeout seconds" 2

  if { [lindex $status 0] != "" } then {
    # Error has already been printed.
    return -1
  }

  if { [lsearch -exact [lreplace $status 0 0] "nodebug"] } then {
    untested "no debugging information for $executable"
    return -1
  }

Or something like that.

How does that look?

Re: [RFC] GDB testsuite patch. (revised version)

[Manoj Iyer]

> gdb_file_cmd can return a positive number like '1' when no debugging
> information is found which can be checked for in gdb.gdb/*.exp and exit
> the testcase. This way the other tests are not effected coz they check for
> $return < 0
>

Sorry abt that ... that was a very bad idea of mine, it will never work.
pls ignore my suggestion.

--
manjo

Re: [RFC] GDB testsuite patch. (revised version)

[Manoj Iyer]

Yes that looks cool. I will wait for you to commit, and I shall send a
patch to that effect.

Thanks Michael
-----
Manjo
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ Cognito ergo sum                                                          +
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

On Fri, 27 Aug 2004, Michael Chastain wrote:

> Manoj Iyer <manjo@austin.ibm.com> wrote:
> > The problem is that there are other tests that run just fine even if no
> > debugging symbols are found. This means if you change gdb_load to check if
> > no debugging symbols are found and return a -ive value this might harm the
> > other testcase.
>
> Right.  You tried that, and Daniel pointed out that it was wrong,
> and I agreed.
>
> > I will be happy to submit that patch, and see if you like my idea.
>
> It's such a good idea, I already started implementing something
> like that.
>
> Here is the new header for gdb_file_cmd:
>
>   # Load a file into the debugger.
>   # The return value is a list with the following information:
>   #
>   #  { message word ... }
>   #
>   # MESSAGE has the following values:
>   #
>   #   ""     file was loaded successfully
>   #   "..."  file was not loaded successfully.
>   #          An error or perror has been generated with MESSAGE.
>   #
>   # If the MESSAGE is "", then there is an optional set of words.
>   # The words may be:
>   #
>   #  nodebug  this file does not contain debug information
>   #
>   # TODO: gdb.base/sepdebug.exp and gdb.stabs/weird.exp might
>   # be able to use this if they can get more information
>   # in the return value.
>
> TCL is all about lists and strings, and it makes life a lot
> easier to return a nice list of strings.
>
> If this still looks like a good idea in a few hours, and I commit it,
> then you can write gdb.gdb/*.exp like this:
>
>   set oldtimeout $timeout
>   set timeout 600
>   verbose "Timeout is now $timeout seconds" 2
>
>   set status [gdb_load $executable]
>
>   set timeout $oldtimeout
>   verbose "Timeout is now $timeout seconds" 2
>
>   if { [lindex $status 0] != "" } then {
>     # Error has already been printed.
>     return -1
>   }
>
>   if { [lsearch -exact [lreplace $status 0 0] "nodebug"] } then {
>     untested "no debugging information for $executable"
>     return -1
>   }
>
> Or something like that.
>
> How does that look?
>