Re: [PATCH] testsuite: Add (extensive) hardware breakpoint testing

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Fri, 11 Nov 2011, Maciej W. Rozycki wrote:

> On Fri, 11 Nov 2011, Doug Evans wrote:
> 
> > {i386,amd64}-linux are important enough targets that I think this
> > should be fixed for 7.4.
> 
>  OK, so we've got this part sorted out -- and I verified the two failures 
> observed are also present with break.exp, so they're a generic 
> x86_64-linux-gnu problem and nothing wrong with this test.
> 
> > One high level comment:
> > There's a lot of basic linespec testing that would be nice to make table driven.
> > E.g., {hbreak,thbreak} x {function, file:lineno, ...}, followed by
> > delete_breakpoints.
> > [I might not do this when testing running to the breakpoints, dunno if
> > that is easily made data-driven, though that would be nice too.]
> > If we're going to do (semi-)exhaustive linespec testing, I'd rather
> > have a table than a lot of repetitive code.
> > 
> > The same applies to break,tbreak.
> > They too could be table driven.
> > 
> > OTOH, maybe it's better to do simple linespec testing separately.
> > There I would move {break,tbreak,hbreak,thbreak} x {function,
> > file:lineno, ...} into its own file,
> > have a table to drive it, and do a lot of testing with less code.
> > It'd be more easily extensible too.
> > 
> > I wouldn't make this a requirement, just mentioning it.
> 
>  I agree lots of older test cases ask for cleaning up -- I refrained from 
> diverging from break.exp too much, although I did some obvious tidy-ups, 
> like removing the remaining send_gdb/gdb_expect pieces in favour to 
> gdb_test/gdb_test_multiple.  Interestingly enough someone did that to 
> break.exp too at one point, but didn't catch all the places.  I think 
> it'll make sense to complete it using hbreak2.exp as a reference for a 
> change.  I can't commit to doing it now, but I'll try as my time permits.
> 
> > > @@ -0,0 +1,629 @@
> > > +# ? Copyright 1988, 1990, 1991, 1992, 1994, 1995, 1996, 1997, 1998, 1999,
> > > +# ? 2000, 2002, 2003, 2007, 2008, 2009, 2010, 2011
> > > +# ? Free Software Foundation, Inc.
> > 
> > This is a new file.  I'm not sure what the rule is, but I think this
> > should just say 2011.
> 
>  I wasn't sure -- the contents of the file certainly are not new, as this 
> is break.exp taken as a whole and then edited.  Not even quite heavily -- 
> you can run a diff or compare the files side by side and they are still 
> pretty close to each other.  I have removed the older years now; they can 
> be still added back if needed.
> 
> > > +# FIXME: The rest of this test doesn't work with anything that can't
> > > +# handle arguments.
> > > +# Huh? ?There doesn't *appear* to be anything that passes arguments
> > > +# below.
> > 
> > I think I'd just remove the FIXME and Huh? comments.
> > And then ideally remove the mips-idt-* test.
> 
>  Again, this was taken from break.exp verbatim.  I have removed it now as 
> it seems to make little sense indeed, but in this case I think the former 
> should be updated accordingly.
> 
> > > + ? ?if [istarget "mips*tx39-*"] {
> > > + ? ? ? set timeout 60
> > 
> > I would do:
> > 
> >   set oldtimeout $timeout
> >   if [istarget ...] {
> >     set timeout 60
> >   }
> > 
> > and then restore timeout at the end of the function.
> 
>  Hmm, the TX39 is an R3000 clone and probably does not have hardware 
> breakpoints in the first place. ;)  So this is another break.exp 
> compatibility place -- I'm leaving it in place for this reason.
> 
>  As to your observation -- does it really matter?  The timeout variable is 
> local to this function (accessed with upvar from gdb_test), so it's wiped 
> away at the point the procedure returns.  I'm therefore leaving it as it 
> is unless you have further notes.
> 
> > > +# Reset the default arguments for VxWorks
> > > +if [istarget "*-*-vxworks*"] {
> > > + ? ?set timeout 10
> > > + ? ?verbose "Timeout is now $timeout seconds" 2
> > 
> > Timeout restoration is no longer needed at the end of a file.
> > I would delete these lines.
> > 
> > > + ? ?gdb_test_no_output "set args main"
> > > +}
> 
>  Hmm, this actually looks like a blind copy&paste from gdb.base/a2-run.exp 
> with the timeout bit added later on.  I have therefore removed the whole 
> conditional -- it's not like we change the arguments anywhere here.
> 
>  Here's an updated version -- any further comments?

 The update below, taken from break.exp, is required to fix:

(gdb) hbreak 999
No line 999 in the current file.
Make hw breakpoint pending on future shared library load? (y or [n]) n
(gdb) FAIL: gdb.base/hbreak2.exp: hardware break on non-existent source line (got interactive prompt)

on some platforms.  I have integrated it into my patch now.

 Any progress with this test case otherwise?

  Maciej

Index: gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/hbreak2.exp
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/testsuite/gdb.base/hbreak2.exp	2011-12-08 15:28:05.585562274 +0000
+++ gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/hbreak2.exp	2011-12-08 15:04:31.305404817 +0000
@@ -304,6 +304,7 @@ if ![runto_main] then { fail "break test
 # Verify that GDB responds gracefully when asked to set a breakpoint
 # on a nonexistent source line.
 #
+gdb_test_no_output "set breakpoint pending off"
 gdb_test "hbreak 999" \
     "No line 999 in file .*" \
     "hardware break on non-existent source line"