[Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Jim Blandy]

[-- Attachment #1: Type: text/plain, Size: 33 bytes --]


Ping on this test suite patch.


[-- Attachment #2: Type: message/rfc822, Size: 4823 bytes --]

From: Jim Blandy <jimb@redhat.com>
To: gdb-patches@sources.redhat.com
Subject: RFA: Check that `Local' is not in scope when it shouldn't be
Date: 14 Nov 2002 15:11:15 -0500
Message-ID: <vt2wunfoqsc.fsf@zenia.red-bean.com>


See http://sources.redhat.com/ml/gdb/2002-11/msg00153.html for the
problem being addressed.  I've filed a new PR for the problem this
patch uncovers, and used its number in the kfail for the new test
here.

gdb/testsuite/ChangeLog:
2002-11-14  Jim Blandy  <jimb@redhat.com>

	* local.exp: Don't expect Local to be in scope in main; it's local
	to foobar.  Check for it there, and check that it's not present in
	main.
	* local.cc (marker2): New function.
	(foobar): Call marker1.
	(main): Call marker2 instead of marker1.

Index: gdb/testsuite/gdb.c++/local.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/local.cc,v
retrieving revision 1.2
diff -c -r1.2 local.cc
*** gdb/testsuite/gdb.c++/local.cc	10 Apr 2002 03:52:21 -0000	1.2
--- gdb/testsuite/gdb.c++/local.cc	14 Nov 2002 20:20:27 -0000
***************
*** 4,9 ****
--- 4,12 ----
  { 
  }
  
+ void marker2 (void)
+ {
+ }
    
  int foobar (int x)
  {
***************
*** 20,25 ****
--- 23,30 ----
    static Local l1;
    char  c;
  
+   marker1 ();
+ 
    l.loc1 = 23;
  
    c = l.loc_foo('x');
***************
*** 56,61 ****
  
     il.ilc = 'b';
     il.ip = &c;
!    marker1();
   }
  }
--- 61,66 ----
  
     il.ilc = 'b';
     il.ip = &c;
!    marker2();
   }
  }
Index: gdb/testsuite/gdb.c++/local.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/local.exp,v
retrieving revision 1.9
diff -c -r1.9 local.exp
*** gdb/testsuite/gdb.c++/local.exp	27 May 2002 18:00:14 -0000	1.9
--- gdb/testsuite/gdb.c++/local.exp	14 Nov 2002 20:20:28 -0000
***************
*** 67,73 ****
      continue
  }
  
! gdb_test "up" ".*main.*" "up from marker1"
  
  # Local classes in g++ get names like "main.1::InnerLocal", just like local
  # static variables.  Some targets use "___" instead of ".".
--- 67,73 ----
      continue
  }
  
! gdb_test "up" ".*foobar.*" "up from marker1"
  
  # Local classes in g++ get names like "main.1::InnerLocal", just like local
  # static variables.  Some targets use "___" instead of ".".
***************
*** 110,115 ****
--- 110,128 ----
    -re ".*$gdb_prompt $"   {  fail "ptype Local" }
    timeout             { fail "(timeout) ptype Local" }
  }
+ 
+ if ![runto 'marker2'] then {
+     perror "couldn't run to marker2"
+     continue
+ }
+ 
+ gdb_test "up" ".*main.*" "up from marker2"
+ 
+ # Make sure that `Local' isn't in scope here; it's local to foobar.
+ # setup_kfail "gdb/825"
+ gdb_test "ptype Local" "No symbol \"Local\" in current context.*" \
+          "Local out of scope (gdb/825)"
+ 
  
  # DTS CLLbs14316 and CLLbs17058
  # coulter - I added a clause for HP's aCC compiler.  We print out the type

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Daniel Jacobowitz]

On Fri, Dec 20, 2002 at 01:36:40PM -0500, Jim Blandy wrote:
> 
> Ping on this test suite patch.

Three things:
  - This is a C++ testsuite patch, it can just go to David.
  - You can uncomment the setup_xfail now.
  - Please include the directory name (gdb.c++) in the ChangeLog
entries.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[David Carlton]

On Fri, 20 Dec 2002 14:10:20 -0500, Daniel Jacobowitz <drow@mvista.com> said:

>   - This is a C++ testsuite patch, it can just go to David.

Right!  And send C++ testsuite patches to Michael, too: he's able to
review them again these days as well.  So hopefully one or the other
of us should be able to get at such patches quickly.

>   - Please include the directory name (gdb.c++) in the ChangeLog
> entries.

Good catch, I missed that one.

David Carlton
carlton@math.stanford.edu

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[David Carlton]

On 20 Dec 2002 13:36:40 -0500, Jim Blandy <jimb@redhat.com> said:

> Ping on this test suite patch.

I'm confused: don't you want to do the first 'ptype Local' _before_
going up from foobar?  In which case your added test might as well
happen after you go up from foobar but before running to marker2.

Also, we're actually kfailing things now, though we weren't when you
first submitted the patch.  So if you could modify the patch to
actually call setup_kfail (which takes two arguments, the second one
of which is presumably "*-*-*"), I'd appreciate it.  What I would
recommend is to replace this

# setup_kfail "gdb/825"
gdb_test "ptype Local" "No symbol \"Local\" in current context.*" \
         "Local out of scope (gdb/825)"

with something like

gdb_send "ptype Local\n"
gdb_expect {
  -re "No symbol \"Local\" in current context.*" {
     setup_kfail "gdb/825" "*-*-*"
     pass "Local out of scope"
  }
  -re "(actual output)" {
     setup_kfail "gdb/825" "*-*-*"
     fail "Local out of scope"
  -re ".*$gdb_prompt $" {
     fail "Local out of scope
  }
  timeout {
    fail "(timeout) Local out of scope"
}

For "actual output", you could either have one block for each of the
outputs (pass or (k)fail) listed in the earlier "ptype Local" test, or
you can just have one test that unifies each of the earlier outputs;
whichever you think is easiest.

This assumes that you think that GDB never gets this right currently;
if you think that GDB sometimes does get it right, then don't put the
setup_kfail before the pass message.

Other than that, it looks good; I certainly like the basic idea.

Also, for what it's worth, I personally prefer diff -up to diff -c.

David Carlton
carlton@math.stanford.edu

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[David Carlton]

On 20 Dec 2002 11:50:55 -0800, David Carlton <carlton@math.Stanford.EDU> said:

>   -re "No symbol \"Local\" in current context.*" {

Actually, this regexp isn't appropriate now that it's not an argument
to gdb_test: either

"No symbol \"Local\" in current context.*$gdb_prompt $"

or the actual output that you expect (maybe just replacing .* by
\r\n?) would be better.

David Carlton
carlton@math.stanford.edu

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Jim Blandy]

Okay, here's a revision of my patch to gdb.c++/local.{c,exp} that
correctly recognizes the two bugs where they occur:
- GDB prints 'class Local' incorrectly
- GDB misunderstands the scope in which 'class Local' is visible
I believe it flags them as known failures in the appropriate way.

David Carlton originally asked:
> I'm confused: don't you want to do the first 'ptype Local' _before_
> going up from foobar?  In which case your added test might as well
> happen after you go up from foobar but before running to marker2.

The test doesn't go up from foobar; it goes up from marker1, which is
called from foobar, so the "up" makes foobar the current scope.

Michael Chastain corrected the way I'd written the second kfail
patch.  I believe I've done it right this time --- so that if the
output changes from the current known incorrect output to anything
other than the correct output, the known failure will become a
straight failure.

I couldn't figure out, though, why folks were advising me to use
setup_kfail with a pattern that always matches, instead of simply
calling kfail directly.  So I just used kfail.

Whip me, beat me, make me write bad checks.

gdb/testsuite/ChangeLog:
2003-02-04  Jim Blandy  <jimb@redhat.com>

	* gdb.c++/local.exp: Don't expect Local to be in scope in main;
	it's local to foobar.  Check for it there, and check that it's not
	present in main.
	* gdb.c++/local.cc (marker2): New function.
	(foobar): Call marker1.
	(main): Call marker2 instead of marker1.

Index: gdb/testsuite/gdb.c++/local.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/local.cc,v
retrieving revision 1.2
diff -c -r1.2 local.cc
*** gdb/testsuite/gdb.c++/local.cc	10 Apr 2002 03:52:21 -0000	1.2
--- gdb/testsuite/gdb.c++/local.cc	4 Feb 2003 21:07:07 -0000
***************
*** 4,9 ****
--- 4,12 ----
  { 
  }
  
+ void marker2 (void)
+ {
+ }
    
  int foobar (int x)
  {
***************
*** 20,25 ****
--- 23,30 ----
    static Local l1;
    char  c;
  
+   marker1 ();
+ 
    l.loc1 = 23;
  
    c = l.loc_foo('x');
***************
*** 56,61 ****
  
     il.ilc = 'b';
     il.ip = &c;
!    marker1();
   }
  }
--- 61,66 ----
  
     il.ilc = 'b';
     il.ip = &c;
!    marker2();
   }
  }
Index: gdb/testsuite/gdb.c++/local.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/local.exp,v
retrieving revision 1.10
diff -c -r1.10 local.exp
*** gdb/testsuite/gdb.c++/local.exp	14 Jan 2003 04:34:46 -0000	1.10
--- gdb/testsuite/gdb.c++/local.exp	4 Feb 2003 21:07:07 -0000
***************
*** 67,73 ****
      continue
  }
  
! gdb_test "up" ".*main.*" "up from marker1"
  
  # Local classes in g++ get names like "main.1::InnerLocal", just like local
  # static variables.  Some targets use "___" instead of ".".
--- 67,73 ----
      continue
  }
  
! gdb_test "up" ".*foobar.*" "up from marker1"
  
  # Local classes in g++ get names like "main.1::InnerLocal", just like local
  # static variables.  Some targets use "___" instead of ".".
***************
*** 102,115 ****
  gdb_expect {
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*\}.*$gdb_prompt $" { pass "ptype Local" }
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*\}.*$gdb_prompt $" {
!     # setup_kfail "gdb/483"
!     fail "ptype Local (gdb/483)"
    }
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*Local & operator *=\\((foobar__Fi${sep}::|)Local const *&\\);\[\r\n\t \]*Local\\((foobar__Fi${sep}::|)Local const *&\\);\[\r\n\t \]*Local\\((void|)\\);\[\r\n\t \]*char loc_foo\\(char\\);\[\r\n\t \]*\}.*$gdb_prompt $" { pass "ptype Local" }
    -re "type = class Local \{\r\n\[\t \]*public:\r\n\[\t \]*int loc1;\r\n\r\n\[\t \]*char loc_foo\\(char\\);\r\n\[\t \]*\\(Local at.*local\\.cc:\[0-9\]*\\)\r\n\}.*$gdb_prompt $" { xpass "ptype Local (old aCC)" }
    -re ".*$gdb_prompt $"   {  fail "ptype Local" }
    timeout             { fail "(timeout) ptype Local" }
  }
  
  # DTS CLLbs14316 and CLLbs17058
  # coulter - I added a clause for HP's aCC compiler.  We print out the type
--- 102,144 ----
  gdb_expect {
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*\}.*$gdb_prompt $" { pass "ptype Local" }
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*char loc_foo\\((const *|)char\\);\[\r\n\t \]*\}.*$gdb_prompt $" {
!     kfail "gdb/483" "ptype Local"
    }
    -re "type = class Local \{\[\r\n\t \]*public:\[\r\n\t \]*int loc1;\[\r\n\t \]*Local & operator *=\\((foobar__Fi${sep}::|)Local const *&\\);\[\r\n\t \]*Local\\((foobar__Fi${sep}::|)Local const *&\\);\[\r\n\t \]*Local\\((void|)\\);\[\r\n\t \]*char loc_foo\\(char\\);\[\r\n\t \]*\}.*$gdb_prompt $" { pass "ptype Local" }
    -re "type = class Local \{\r\n\[\t \]*public:\r\n\[\t \]*int loc1;\r\n\r\n\[\t \]*char loc_foo\\(char\\);\r\n\[\t \]*\\(Local at.*local\\.cc:\[0-9\]*\\)\r\n\}.*$gdb_prompt $" { xpass "ptype Local (old aCC)" }
    -re ".*$gdb_prompt $"   {  fail "ptype Local" }
    timeout             { fail "(timeout) ptype Local" }
  }
+ 
+ gdb_test "break marker2"
+ gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker2.*" \
+     "continuing to marker2"
+ 
+ gdb_test "up" ".*main.*" "up from marker2"
+ 
+ # Make sure that `Local' isn't in scope here; it's local to foobar.
+ # setup_kfail "gdb/825"
+ send_gdb "ptype Local\n"
+ set eol "\[\t \]*\[\r\n\]+\[\t \]*"
+ gdb_expect {
+     -re "No symbol \"Local\" in current context.*${gdb_prompt} $" {
+         pass "Local out of scope"
+     }
+     -re "ptype Local${eol}type = class Local {${eol}  public:${eol}    int loc1;${eol}${eol}    Local & operator=\\(Local const&\\);${eol}    Local\\(Local const&\\);${eol}    Local\\(\\);${eol}    char loc_foo\\(char\\);${eol}}${eol}${gdb_prompt} " {
+         # GCC emits STABS debugging information in a way that doesn't
+         # properly preserve the scoping of local classes.  I think
+         # we'd need to start using Sun's extensions to stabs to get
+         # this right.
+         kfail gdb/825 "Local out of scope"
+     }
+     -re ".*${gdb_prompt} $" {
+         fail "Local out of scope"
+     }
+     timeout {
+         fail "Local out of scope (timeout)"
+     }
+ }
+         
  
  # DTS CLLbs14316 and CLLbs17058
  # coulter - I added a clause for HP's aCC compiler.  We print out the type

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[David Carlton]

On 04 Feb 2003 16:53:48 -0500, Jim Blandy <jimb@redhat.com> said:
> David Carlton originally asked:

>> I'm confused: don't you want to do the first 'ptype Local' _before_
>> going up from foobar?  In which case your added test might as well
>> happen after you go up from foobar but before running to marker2.

> The test doesn't go up from foobar; it goes up from marker1, which is
> called from foobar, so the "up" makes foobar the current scope.

Oh, duh, sorry about that.

> Michael Chastain corrected the way I'd written the second kfail
> patch.  I believe I've done it right this time --- so that if the
> output changes from the current known incorrect output to anything
> other than the correct output, the known failure will become a
> straight failure.

Right.

> I couldn't figure out, though, why folks were advising me to use
> setup_kfail with a pattern that always matches, instead of simply
> calling kfail directly.  So I just used kfail.

Yes, kfail is better.

Approved; thanks for taking care of this.  My list of
non-{PASS,KFAIL}s in gdb.c++ is dwindling nicely.

David Carlton
carlton@math.stanford.edu

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Jim Blandy]

David Carlton <carlton@math.stanford.edu> writes:
> Approved; thanks for taking care of this.  My list of
> non-{PASS,KFAIL}s in gdb.c++ is dwindling nicely.

Committed --- thanks.

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Michael Elizabeth Chastain]

David C writes:
> Right!  And send C++ testsuite patches to Michael, too: he's able to
> review them again these days as well.  So hopefully one or the other
> of us should be able to get at such patches quickly.

I'll be wearing my C++ test suite maintainer hat on Sunday I think.

Michael C

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Michael Elizabeth Chastain]

Regarding this patch:

  2002-11-14  Jim Blandy  <jimb@redhat.com>

	* local.exp: Don't expect Local to be in scope in main; it's local
	to foobar.  Check for it there, and check that it's not present in
	main.
	* local.cc (marker2): New function.
	(foobar): Call marker1.
	(main): Call marker2 instead of marker1.

The first part of this patch, "Don't expect Local to be in scope in main;
it's local to foobar.", is approved.  I agree with your analysis and most
of your solution.  I don't like the second call to "runto 'marker2'",
because "runto" restarts gdb needlessly.  I think it's better to do:

  gdb_test "break marker2" ...
  gdb_test "continue" "Break.* marker2 ... at ..."

But the test works correctly as you wrote it, and the existing code 
doesn't, so I approve it and someone can rewrite the second "runto"
after you commit this.

The second part, "... check that it's not present in main",
is not approved.  You wrote the KFAIL like this:

  # setup_kfail "gdb/825"
  gdb_test "ptype Local" "No symbol \"Local\" in current context.*" \
    "Local out of scope (gdb/825)"

There is a coding style issue here.  My vision is that KFAIL's are always
a response to specific known bad output from gdb, like this:

  send_gdb "ptype Local\n"
  gdb_expect {
    -re "No symbol \"Local in current context.\r\n$gdb_prompt $" {
      pass "ptype Local"
    }
    -re "... big regular expression that matches the correct declaration of Local from the wrong scope ..." {
      # comment here about which configurations we expect this for
      kfail "gdb/825" "ptype Local"
    }
    -re ".*$gdb_prompt $" {
      fail "ptype Local"
    }
    timeout {
      fail "ptype Local (timeout)"
    }
  }

This way, the test can still generate FAIL if gdb starts acting in a
new unknown way.  The first test actually does this; it went from FAIL
(gdb/483) to an unmarked FAIL, which indicated that there was a problem
(which turned out to be in the test script itself).

I think this issue needs some discussion and then a decision from
FernandoN or AndrewC so I'd like to hold off on the second test in
this patch while we resolve this.

(I spent too much time on this patch because I started doing my own
update of Local.exp with my style of KFAIL's everywhere.  Then I decided
it's better just to respond to JimB and work on an all-updated local.exp
later.)

Michael C

Re: [Jim Blandy <jimb@redhat.com>] RFA: Check that `Local' is not in scope when it shouldn't be

[Michael Elizabeth Chastain]

Looks okay to me too, I'll be catching it in the next spin.

JimB> I couldn't figure out, though, why folks were advising me to use
JimB> setup_kfail with a pattern that always matches, instead of simply
JimB> calling kfail directly.  So I just used kfail.

Your intuition is proper.

I wrote the 'setup_kfail' comments before kfail was implemented,
and before we'd figured out our idioms for it.  It turns out that we
will have lots of 'kfail' and very few 'setup_kfail'.  I think the
remaining xfail's should be that way too (after we kill all the bogus
ones).

Michael C