Patch for PR 9399

Patch for PR 9399

[Chris Moller]

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

The attached stuff is a patch for PR9399:

http://sourceware.org/bugzilla/show_bug.cgi?id=9399  "gdb can't call or 
print a const function that uses virtual inheritance"

The bug was in valops.c:value_cast_structs() which could do spurious 
upcasts or downcasts.  The fix tests whether any casts at all are 
necessary and, if not, returns a NULL.

The patch hits two ChangeLogs, one for the fix, in .../gdb/ChangeLog, 
the other for the testcase in gdb/testsuite/ChangeLog.  Respectively, 
they are:

    2009-12-08  Chris Moller <cmoller@redhat.com>

            PR gdb/9399
            * valops.c (value_cast_structs): Added test to return NULL if no
            casting needed.


    2009-12-08  Chris Moller <cmoller@redhat.com>

           PR gdb/9399
           * gdb.cp/virtfunc2.exp:
           * gdb.cp/virtfunc2.cc: New tests for a patch to fix PR gd/9399
           * gdb.cp/Makefile.in: Added tests to EXECUTABLES

(These are both included in the attached patch file.)

The patch file includes the patch to gdb/valops.c, gdb/ChangeLog, 
gdb/testsuite/ChangeLog, gdb/testsuite/gdb.cp/Makefile.in, and 
mc-log.diffs.  (The patch to  Makefile.in is to incorporate the testcase 
for 'make check'; mc-log.diffs are the diffs between before and after 
runs of 'make check')  I can't do a 'cvs add' so the the testcase 
expects and .cc file are attached separately as 
gdb/testsuite/gdb.cp/virtfunc2.cc and gdb/testsuite/gdb.cp/virtfunc2.exp.

You can see what the patch does by compiling -g virtfunc.cc, gdb-ing it, 
breaking in the return stmt at // marker1, and doing things like "print 
o.do_print()".  Without the patch, gdb tries to access location 0x0; 
with the patch it does the right thing.  (There are more tests in 
virtfunc2.exp)

I've included the diff between the before and after 'make check' logs.  
I don't think the patch breaks anything--most of the diffs are with 
respect to the different directories used for before and after builds.  
Starting at line 131 in mc-log.diffs, you can see the new tests, 
virtfunc2, and, following that, the six new expected passes.

I've assigned that bug to myself.  If you're happy with the patch, let 
me know and I'll close the bug.  (Or do whatever you usually do...)

Have fun,
Chris Moller
Red Hat



[-- Attachment #2: pr9399.patch --]
[-- Type: text/plain, Size: 3626 bytes --]

? gdb/testsuite/gdb.cp/virtfunc2.cc
? gdb/testsuite/gdb.cp/virtfunc2.exp
Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10874.2.54
diff -c -p -r1.10874.2.54 ChangeLog
*** gdb/ChangeLog	24 Nov 2009 17:23:03 -0000	1.10874.2.54
--- gdb/ChangeLog	9 Dec 2009 13:00:50 -0000
***************
*** 1,3 ****
--- 1,9 ----
+ 2009-12-08  Chris Moller  <cmoller@redhat.com>
+ 
+ 	PR gdb/9399
+ 	* valops.c (value_cast_structs): Added test to return NULL if no
+ 	casting needed.
+ 
  2009-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
  
  	* dwarf2read.c [HAVE_MMAP] (MAP_FAILED): Define if missing.
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.225.2.1
diff -c -p -r1.225.2.1 valops.c
*** gdb/valops.c	29 Sep 2009 00:41:24 -0000	1.225.2.1
--- gdb/valops.c	9 Dec 2009 13:00:52 -0000
*************** value_cast_structs (struct type *type, s
*** 232,237 ****
--- 232,246 ----
  	       || TYPE_CODE (t2) == TYPE_CODE_UNION)
  	      && !!"Precondition is that value is of STRUCT or UNION kind");
  
+   /* Check to see if any kind of cast is necessary; if not, punt.
+      (Specifically not using strcmp_iw() because no other comparisons against
+      TYPE_NAME() use it.  They all use standard strcmp() instead.) */
+ 
+   if ((TYPE_NAME (t1) != NULL) &&
+       (TYPE_NAME (t2) != NULL) &&
+       !strcmp (TYPE_NAME (t1), TYPE_NAME (t2)))
+     return NULL;
+ 
    /* Upcasting: look in the type of the source to see if it contains the
       type of the target as a superclass.  If so, we'll need to
       offset the pointer rather than just change its type.  */
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.1960.2.11
diff -c -p -r1.1960.2.11 ChangeLog
*** gdb/testsuite/ChangeLog	29 Sep 2009 19:27:16 -0000	1.1960.2.11
--- gdb/testsuite/ChangeLog	9 Dec 2009 13:01:07 -0000
***************
*** 1,3 ****
--- 1,10 ----
+ 2009-12-08  Chris Moller  <cmoller@redhat.com>
+ 
+ 	PR gdb/9399
+ 	* gdb.cp/virtfunc2.exp:
+ 	* gdb.cp/virtfunc2.cc: New tests for a patch to fix PR gd/9399
+ 	* gdb.cp/Makefile.in: Added tests to EXECUTABLES
+ 
  2009-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
  
  	* gdb.base/breakpoint-shadow.exp: Move the ia64 part into ...
Index: gdb/testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.6
diff -c -p -r1.6 Makefile.in
*** gdb/testsuite/gdb.cp/Makefile.in	15 Jun 2009 12:11:37 -0000	1.6
--- gdb/testsuite/gdb.cp/Makefile.in	9 Dec 2009 13:01:09 -0000
*************** srcdir = @srcdir@
*** 4,10 ****
  EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
  	derivation inherit local member-ptr method misc \
          overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
! 	ref-types ref-params method2 pr9594 gdb2495
  
  all info install-info dvi install uninstall installcheck check:
  	@echo "Nothing to be done for $@..."
--- 4,10 ----
  EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
  	derivation inherit local member-ptr method misc \
          overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
! 	ref-types ref-params method2 pr9594 gdb2495 virtfunc2
  
  all info install-info dvi install uninstall installcheck check:
  	@echo "Nothing to be done for $@..."

[-- Attachment #3: virtfunc2.cc --]
[-- Type: text/x-c++src, Size: 1173 bytes --]

/* This test script is part of GDB, the GNU debugger.

   Copyright 1993, 1994, 1997, 1998, 1999, 2003, 2004, 2009
   Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.
   */

class interface
{
  virtual const int do_print3() const { return 111111; }
};
 
class Obj : virtual public interface
{
public:
  virtual const int do_print() const { return 123456; }
};

class Obj2 : Obj,  virtual public interface
{
  virtual const int do_print2() const { return 654321; }
};

int main(int argc, char** argv) {
  Obj o;
  Obj2 o2;
  return 0;	// marker 1
}

[-- Attachment #4: virtfunc2.exp --]
[-- Type: text/plain, Size: 1766 bytes --]

# Copyright 1992, 1994, 1995, 1996, 1997, 1998, 1999, 2001, 2002, 2003, 2004,
# 2006, 2007, 2008, 2009 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

# This file was written by Chris Moller <moller@redhat.com> based on
# virtfunc.exp

set nl		"\[\r\n\]+"

if { [skip_cplus_tests] } { continue }

load_lib "cp-support.exp"

set testfile "virtfunc2"
set srcfile ${testfile}.cc
set binfile ${objdir}/${subdir}/${testfile}

if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
     untested virtfunc2.exp
     return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

if ![runto_main] then {
    perror "couldn't run to breakpoint"
    continue
}

# set a breakpoint at the return stmt

gdb_breakpoint [gdb_get_line_number "marker 1"]
gdb_continue_to_breakpoint "marker 1"

gdb_test "print o.do_print()"  "\\$\[0-9\]+ = 123456"
gdb_test "print o.do_print3()"  "\\$\[0-9\]+ = 111111"

gdb_test "print o2.do_print()"  "\\$\[0-9\]+ = 123456"
gdb_test "print o2.do_print2()"  "\\$\[0-9\]+ = 654321"
gdb_test "print o2.do_print3()"  "\\$\[0-9\]+ = 111111"


gdb_exit
return 0


[-- Attachment #5: mc-log.diffs --]
[-- Type: text/plain, Size: 13348 bytes --]

1,2c1,61
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite
< make[1]: Entering directory `/home/moller/tinkering/fsfgdb/build/gdb/testsuite'
---
> make[1]: Entering directory `/home/moller/tinkering/fsfgdb/build2'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> make  check-recursive
> make[3]: Entering directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> Making check in doc
> make[4]: Entering directory `/home/moller/tinkering/fsfgdb/build2/bfd/doc'
> make[4]: Nothing to be done for `check'.
> make[4]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/bfd/doc'
> Making check in po
> make[4]: Entering directory `/home/moller/tinkering/fsfgdb/build2/bfd/po'
> make[4]: Nothing to be done for `check'.
> make[4]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/bfd/po'
> make[4]: Entering directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> make[4]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> make[3]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/bfd'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/opcodes'
> Making check in .
> make[3]: Entering directory `/home/moller/tinkering/fsfgdb/build2/opcodes'
> make[3]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/opcodes'
> Making check in po
> make[3]: Entering directory `/home/moller/tinkering/fsfgdb/build2/opcodes/po'
> make[3]: Nothing to be done for `check'.
> make[3]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/opcodes/po'
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/opcodes'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/etc'
> make[2]: Nothing to be done for `check'.
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/etc'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/intl'
> make[2]: Nothing to be done for `check'.
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/intl'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/libdecnumber'
> make[2]: Nothing to be done for `check'.
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/libdecnumber'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/libiberty'
> /home/moller/tinkering/fsfgdb/build2/libiberty/testsuite
> make[3]: Entering directory `/home/moller/tinkering/fsfgdb/build2/libiberty/testsuite'
> gcc -DHAVE_CONFIG_H -g -O2 -I.. -I../../../src/libiberty/testsuite/../../include  -o test-demangle \
> 		../../../src/libiberty/testsuite/test-demangle.c ../libiberty.a
> ./test-demangle < ../../../src/libiberty/testsuite/demangle-expected
> ./test-demangle: 777 tests, 0 failures
> gcc -DHAVE_CONFIG_H -g -O2 -I.. -I../../../src/libiberty/testsuite/../../include  -DHAVE_CONFIG_H -I.. -o test-pexecute \
> 		../../../src/libiberty/testsuite/test-pexecute.c ../libiberty.a
> ./test-pexecute
> gcc -DHAVE_CONFIG_H -g -O2 -I.. -I../../../src/libiberty/testsuite/../../include  -DHAVE_CONFIG_H -I.. -o test-expandargv \
> 		../../../src/libiberty/testsuite/test-expandargv.c ../libiberty.a
> ./test-expandargv
> PASS: test-expandargv-0.
> PASS: test-expandargv-1.
> PASS: test-expandargv-2.
> PASS: test-expandargv-3.
> make[3]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/libiberty/testsuite'
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/libiberty'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/readline'
> make[2]: Nothing to be done for `check'.
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/readline'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/sim'
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/sim'
> make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build2/gdb'
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite
> make[3]: Entering directory `/home/moller/tinkering/fsfgdb/build2/gdb/testsuite'
8c67
< make[2]: Entering directory `/home/moller/tinkering/fsfgdb/build/gdb/testsuite'
---
> make[4]: Entering directory `/home/moller/tinkering/fsfgdb/build2/gdb/testsuite'
13c72
< Test Run By moller on Tue Dec  8 19:39:15 2009
---
> Test Run By moller on Tue Dec  8 21:11:50 2009
145c204
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
147c206
< make[2]: *** [check-gdb.base1] Error 1
---
> make[4]: *** [check-gdb.base1] Error 1
150c209
< Test Run By moller on Tue Dec  8 19:40:53 2009
---
> Test Run By moller on Tue Dec  8 21:13:29 2009
262c321
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
264c323
< make[2]: *** [check-gdb.base2] Error 1
---
> make[4]: *** [check-gdb.base2] Error 1
267c326
< Test Run By moller on Tue Dec  8 19:42:19 2009
---
> Test Run By moller on Tue Dec  8 21:14:56 2009
333c392
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
337c396
< Test Run By moller on Tue Dec  8 19:42:20 2009
---
> Test Run By moller on Tue Dec  8 21:14:56 2009
387c446
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
391c450
< Test Run By moller on Tue Dec  8 19:42:22 2009
---
> Test Run By moller on Tue Dec  8 21:14:58 2009
408c467
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
412c471
< Test Run By moller on Tue Dec  8 19:42:22 2009
---
> Test Run By moller on Tue Dec  8 21:14:59 2009
443c502
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
447c506
< Test Run By moller on Tue Dec  8 19:42:23 2009
---
> Test Run By moller on Tue Dec  8 21:14:59 2009
528a588
> Running ../../../src/gdb/testsuite/gdb.cp/virtfunc2.exp ...
532c592
< # of expected passes		2003
---
> # of expected passes		2009
535c595
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
537c597
< make[2]: *** [check-gdb.cp] Error 1
---
> make[4]: *** [check-gdb.cp] Error 1
540c600
< Test Run By moller on Tue Dec  8 19:42:50 2009
---
> Test Run By moller on Tue Dec  8 21:15:27 2009
573c633
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
577c637
< Test Run By moller on Tue Dec  8 19:42:50 2009
---
> Test Run By moller on Tue Dec  8 21:15:27 2009
612c672
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
616c676
< Test Run By moller on Tue Dec  8 19:42:52 2009
---
> Test Run By moller on Tue Dec  8 21:15:30 2009
640c700
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
644c704
< Test Run By moller on Tue Dec  8 19:42:53 2009
---
> Test Run By moller on Tue Dec  8 21:15:30 2009
664c724
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
668c728
< Test Run By moller on Tue Dec  8 19:42:55 2009
---
> Test Run By moller on Tue Dec  8 21:15:32 2009
690c750
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
694c754
< Test Run By moller on Tue Dec  8 19:42:56 2009
---
> Test Run By moller on Tue Dec  8 21:15:33 2009
725d784
< FAIL: gdb.mi/mi-nsmoribund.exp: unexpected stop
770,771c829
< # of expected passes		1575
< # of unexpected failures	1
---
> # of expected passes		1576
775c833
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
777d834
< make[2]: *** [check-gdb.mi] Error 1
780c837
< Test Run By moller on Tue Dec  8 19:43:26 2009
---
> Test Run By moller on Tue Dec  8 21:16:03 2009
797c854
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
801c858
< Test Run By moller on Tue Dec  8 19:43:26 2009
---
> Test Run By moller on Tue Dec  8 21:16:04 2009
820c877
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
824c881
< Test Run By moller on Tue Dec  8 19:43:27 2009
---
> Test Run By moller on Tue Dec  8 21:16:04 2009
846c903
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
850c907
< Test Run By moller on Tue Dec  8 19:43:28 2009
---
> Test Run By moller on Tue Dec  8 21:16:06 2009
872c929
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
876c933
< Test Run By moller on Tue Dec  8 19:43:28 2009
---
> Test Run By moller on Tue Dec  8 21:16:06 2009
900c957
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
904c961
< Test Run By moller on Tue Dec  8 19:43:33 2009
---
> Test Run By moller on Tue Dec  8 21:16:10 2009
929c986
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
933c990
< Test Run By moller on Tue Dec  8 19:43:33 2009
---
> Test Run By moller on Tue Dec  8 21:16:10 2009
953c1010
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
957c1014
< Test Run By moller on Tue Dec  8 19:43:36 2009
---
> Test Run By moller on Tue Dec  8 21:16:13 2009
976c1033
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
980c1037
< Test Run By moller on Tue Dec  8 19:43:37 2009
---
> Test Run By moller on Tue Dec  8 21:16:14 2009
1037c1094
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
1039c1096
< make[2]: *** [check-gdb.threads] Error 1
---
> make[4]: *** [check-gdb.threads] Error 1
1042c1099
< Test Run By moller on Tue Dec  8 19:44:54 2009
---
> Test Run By moller on Tue Dec  8 21:17:32 2009
1074c1131
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
1078c1135
< Test Run By moller on Tue Dec  8 19:44:57 2009
---
> Test Run By moller on Tue Dec  8 21:17:35 2009
1100c1157
< /home/moller/tinkering/fsfgdb/build/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
---
> /home/moller/tinkering/fsfgdb/build2/gdb/testsuite/../../gdb/gdb version  7.0.0.20091209-cvs -nw -nx 
1102,1103c1159,1163
< make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build/gdb/testsuite'
< make[1]: Leaving directory `/home/moller/tinkering/fsfgdb/build/gdb/testsuite'
---
> make[4]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/gdb/testsuite'
> make[3]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/gdb/testsuite'
> make[2]: Leaving directory `/home/moller/tinkering/fsfgdb/build2/gdb'
> make[1]: Nothing to be done for `check-target'.
> make[1]: Leaving directory `/home/moller/tinkering/fsfgdb/build2'

Re: Patch for PR 9399

[Daniel Jacobowitz]

On Wed, Dec 09, 2009 at 08:32:34AM -0500, Chris Moller wrote:
> The patch file includes the patch to gdb/valops.c, gdb/ChangeLog,
> gdb/testsuite/ChangeLog, gdb/testsuite/gdb.cp/Makefile.in, and
> mc-log.diffs.  (The patch to  Makefile.in is to incorporate the
> testcase for 'make check'; mc-log.diffs are the diffs between before
> and after runs of 'make check')

Next time, please diff -u gdb.sum files; the rest is just noise.
Those do vary a little bit but they're more stable.  It looks like
your only change is mi-nsmoribund.exp, which is sometimes flaky.

> I can't do a 'cvs add' so the the
> testcase expects and .cc file are attached separately as
> gdb/testsuite/gdb.cp/virtfunc2.cc and
> gdb/testsuite/gdb.cp/virtfunc2.exp.

Take a look at cvsutils; "cvsdo add" works.

(Or, nowadays, I suppose you could use the git mirror! :-)

> You can see what the patch does by compiling -g virtfunc.cc, gdb-ing
> it, breaking in the return stmt at // marker1, and doing things like
> "print o.do_print()".  Without the patch, gdb tries to access
> location 0x0; with the patch it does the right thing.  (There are
> more tests in virtfunc2.exp)

Where does the access to 0x0 come from?  Is it inside
search_struct_field?  I wouldn't expect value_cast_structs to do any
cast in this case, but it does do a little extra work.

> +   if ((TYPE_NAME (t1) != NULL) &&
> +       (TYPE_NAME (t2) != NULL) &&
> +       !strcmp (TYPE_NAME (t1), TYPE_NAME (t2)))

&& on the beginning of the line, please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch for PR 9399

[Chris Moller]

On 12/09/09 09:05, Daniel Jacobowitz wrote:
> On Wed, Dec 09, 2009 at 08:32:34AM -0500, Chris Moller wrote:
>
>> The patch file includes the patch to gdb/valops.c, gdb/ChangeLog,
>> gdb/testsuite/ChangeLog, gdb/testsuite/gdb.cp/Makefile.in, and
>> mc-log.diffs.  (The patch to  Makefile.in is to incorporate the
>> testcase for 'make check'; mc-log.diffs are the diffs between before
>> and after runs of 'make check')
>>
>
> Next time, please diff -u gdb.sum files; the rest is just noise.
>

Okay--if I ever do this again. :-)

> Those do vary a little bit but they're more stable.  It looks like
> your only change is mi-nsmoribund.exp, which is sometimes flaky.
>
>
>> I can't do a 'cvs add' so the the
>> testcase expects and .cc file are attached separately as
>> gdb/testsuite/gdb.cp/virtfunc2.cc and
>> gdb/testsuite/gdb.cp/virtfunc2.exp.
>>
>
> Take a look at cvsutils; "cvsdo add" works.
>
> (Or, nowadays, I suppose you could use the git mirror! :-)
>

Didn't know you had a git mirror--I just pulled the CVS version because 
it was pointed to on http://sourceware.org/gdb/

>
>> You can see what the patch does by compiling -g virtfunc.cc, gdb-ing
>> it, breaking in the return stmt at // marker1, and doing things like
>> "print o.do_print()".  Without the patch, gdb tries to access
>> location 0x0; with the patch it does the right thing.  (There are
>> more tests in virtfunc2.exp)
>>
>
> Where does the access to 0x0 come from?  Is it inside
> search_struct_field?

Ultimately, yes.  Without the patch, the thread ultimately gets to

      if (BASETYPE_VIA_VIRTUAL (type, i))

in search_struct_field and then to the memcpy about 30 lines later that 
extracts a new value struct.  That main_type of that value doesn't 
include a field for the virtual function, so it's never found, and 
ultimately returns a null pointer.  I haven't a clue why it works that 
way--for a while I was working on the assumption that the DWARF reader 
was screwing up, but if it is, it's too subtle for me.

>   I wouldn't expect value_cast_structs to do any
> cast in this case,

value_cast_structs only does nothing if both TYPE_NAME()s are null.  I 
was wondering if, back when the code was originally written, if there 
never was a case when both TYPE_NAME()s were non-null--it's the only 
way, other than simple oversight, I can account for that case not being 
covered.

> but it does do a little extra work.
>
>
>> +   if ((TYPE_NAME (t1) != NULL)&&
>> +       (TYPE_NAME (t2) != NULL)&&
>> +       !strcmp (TYPE_NAME (t1), TYPE_NAME (t2)))
>>
>
> &&  on the beginning of the line, please.
>
>

Re: Patch for PR 9399

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> The attached stuff is a patch for PR9399:

Chris> I've assigned that bug to myself.  If you're happy with the patch, let
Chris> me know and I'll close the bug.  (Or do whatever you usually do...)

Yeah, what I do is set the target milestone to 7.1 and close as FIXED
after the patch is committed.

Chris> +   /* Check to see if any kind of cast is necessary; if not, punt.
Chris> +      (Specifically not using strcmp_iw() because no other comparisons against
Chris> +      TYPE_NAME() use it.  They all use standard strcmp() instead.) */

Delete this comment.

Chris> /* This test script is part of GDB, the GNU debugger.

Chris>    Copyright 1993, 1994, 1997, 1998, 1999, 2003, 2004, 2009

Just 2009.

Chris> # Copyright 1992, 1994, 1995, 1996, 1997, 1998, 1999, 2001, 2002, 2003, 2004,
Chris> # 2006, 2007, 2008, 2009 Free Software Foundation, Inc.

Likewise.

This is ok with those changes.  Thanks.

Tom

Re: Patch for PR 9399

[Chris Moller]

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

I fixed all the stuff you mention below, plus I added an FSF copyright 
notice to virtfunc2.cc--somehow, I forgot to do that yesterday--and 
fixed the fix to valops.c.  (The original version worked in archer--more 
or less by pure accident, I suspect--but failed in FSF.--I'd forgotten 
to make sure that type names were non-null before calling strcmp on 
them.  Fixed now.)

I've attached the new patch file and virtfunc2.* files.

On 12/10/09 11:59, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  The attached stuff is a patch for PR9399:
>
> Chris>  I've assigned that bug to myself.  If you're happy with the patch, let
> Chris>  me know and I'll close the bug.  (Or do whatever you usually do...)
>
> Yeah, what I do is set the target milestone to 7.1 and close as FIXED
> after the patch is committed.
>
> Chris>  +   /* Check to see if any kind of cast is necessary; if not, punt.
> Chris>  +      (Specifically not using strcmp_iw() because no other comparisons against
> Chris>  +      TYPE_NAME() use it.  They all use standard strcmp() instead.) */
>
> Delete this comment.
>
> Chris>  /* This test script is part of GDB, the GNU debugger.
>
> Chris>     Copyright 1993, 1994, 1997, 1998, 1999, 2003, 2004, 2009
>
> Just 2009.
>
> Chris>  # Copyright 1992, 1994, 1995, 1996, 1997, 1998, 1999, 2001, 2002, 2003, 2004,
> Chris>  # 2006, 2007, 2008, 2009 Free Software Foundation, Inc.
>
> Likewise.
>
> This is ok with those changes.  Thanks.
>
> Tom
>    


[-- Attachment #2: pr9399.patch --]
[-- Type: text/plain, Size: 2077 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3513692..3d999e3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2009-12-08  Chris Moller  <cmoller@redhat.com>
+
+	PR gdb/9399
+	* valops.c (value_cast_structs): Added test to return NULL if no
+	casting needed.
+
 2009-09-16  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR gdb/10649
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b9039df..c00e954 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2009-12-08  Chris Moller  <cmoller@redhat.com>
+
+	PR gdb/9399
+	* gdb.cp/virtfunc2.exp: New tests
+	* gdb.cp/virtfunc2.cc: New tests
+	* gdb.cp/Makefile.in: Added tests to EXECUTABLES
+
 2009-09-15  Tom Tromey  <tromey@redhat.com>
 
 	* lib/mi-support.exp (mi_create_varobj): Update.
diff --git a/gdb/testsuite/gdb.cp/Makefile.in b/gdb/testsuite/gdb.cp/Makefile.in
index 0a087c7..c990a64 100644
--- a/gdb/testsuite/gdb.cp/Makefile.in
+++ b/gdb/testsuite/gdb.cp/Makefile.in
@@ -4,7 +4,7 @@ srcdir = @srcdir@
 EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
 	derivation inherit local member-ptr method misc \
         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
-	ref-types ref-params method2 pr9594 gdb2495
+	ref-types ref-params method2 pr9594 gdb2495 virtfunc2
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
diff --git a/gdb/valops.c b/gdb/valops.c
index 3ad54ec..de24a68 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -232,6 +232,11 @@ value_cast_structs (struct type *type, struct value *v2)
 	       || TYPE_CODE (t2) == TYPE_CODE_UNION)
 	      && !!"Precondition is that value is of STRUCT or UNION kind");
 
+  if ((TYPE_NAME (t1) != NULL)
+      && (TYPE_NAME (t2) != NULL)
+      && !strcmp (TYPE_NAME (t1), TYPE_NAME (t2)))
+    return NULL;
+
   /* Upcasting: look in the type of the source to see if it contains the
      type of the target as a superclass.  If so, we'll need to
      offset the pointer rather than just change its type.  */

[-- Attachment #3: virtfunc2.cc --]
[-- Type: text/x-c++src, Size: 1096 bytes --]

 /* This test script is part of GDB, the GNU debugger.

   Copyright 2009
   Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.
   */

class interface
{
  virtual int do_print3() { return 111111; }
};
 
class Obj : virtual public interface
{
public:
  virtual int do_print() { return 123456; }
};

class Obj2 : Obj,  virtual public interface
{
  virtual int do_print2() { return 654321; }
};

int main(int argc, char** argv) {
  Obj o;
  Obj2 o2;
  return 0;	// marker 1
}

[-- Attachment #4: virtfunc2.exp --]
[-- Type: text/plain, Size: 1680 bytes --]

# Copyright 2009 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

# This file was written by Chris Moller <moller@redhat.com> based on
# virtfunc.exp

set nl		"\[\r\n\]+"

if { [skip_cplus_tests] } { continue }

load_lib "cp-support.exp"

set testfile "virtfunc2"
set srcfile ${testfile}.cc
set binfile ${objdir}/${subdir}/${testfile}

if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
     untested virtfunc2.exp
     return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

if ![runto_main] then {
    perror "couldn't run to breakpoint"
    continue
}

# set a breakpoint at the return stmt

gdb_breakpoint [gdb_get_line_number "marker 1"]
gdb_continue_to_breakpoint "marker 1"

gdb_test "print o.do_print()"  "\\$\[0-9\]+ = 123456"
gdb_test "print o.do_print3()"  "\\$\[0-9\]+ = 111111"

gdb_test "print o2.do_print()"  "\\$\[0-9\]+ = 123456"
gdb_test "print o2.do_print2()"  "\\$\[0-9\]+ = 654321"
gdb_test "print o2.do_print3()"  "\\$\[0-9\]+ = 111111"


gdb_exit
return 0

Re: Patch for PR 9399

[Daniel Jacobowitz]

Tom's approved this.  That's fine.  But for posterity:

On Wed, Dec 09, 2009 at 10:10:44AM -0500, Chris Moller wrote:
> >>You can see what the patch does by compiling -g virtfunc.cc, gdb-ing
> >>it, breaking in the return stmt at // marker1, and doing things like
> >>"print o.do_print()".  Without the patch, gdb tries to access
> >>location 0x0; with the patch it does the right thing.  (There are
> >>more tests in virtfunc2.exp)
> >>
> >
> >Where does the access to 0x0 come from?  Is it inside
> >search_struct_field?
> 
> Ultimately, yes.  Without the patch, the thread ultimately gets to
> 
>      if (BASETYPE_VIA_VIRTUAL (type, i))
> 
> in search_struct_field and then to the memcpy about 30 lines later
> that extracts a new value struct.  That main_type of that value
> doesn't include a field for the virtual function, so it's never
> found, and ultimately returns a null pointer.  I haven't a clue why
> it works that way--for a while I was working on the assumption that
> the DWARF reader was screwing up, but if it is, it's too subtle for
> me.

I couldn't follow this and haven't had time to debug it myself yet.
But we shouldn't dereference any null (target) pointers in this.  If
we do, there's a bug in some routine this function calls.

> >  I wouldn't expect value_cast_structs to do any
> >cast in this case,
> 
> value_cast_structs only does nothing if both TYPE_NAME()s are null.
> I was wondering if, back when the code was originally written, if
> there never was a case when both TYPE_NAME()s were non-null--it's the
> only way, other than simple oversight, I can account for that case
> not being covered.

No, it looks exactly like I intended.  If we have two classes, we
check if A is a base class of B, then if B is a base class of A.
If A == B, neither of those checks will be true, and we'll return
without doing anything.

So the problem is that one of those base class checks is calling
error().  That's not supposed to happen.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch for PR 9399

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> +  if ((TYPE_NAME (t1) != NULL)
Chris> +      && (TYPE_NAME (t2) != NULL)

There are too many parens here.
It should be

  if (TYPE_NAME (t1) != NULL
      && TYPE_NAME (t2) != NULL
      ...

You don't have to resubmit this, just fix it and check it in.

thanks,
Tom

Re: Patch for PR 9399

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> Tom's approved this.  That's fine.

I'm open to unapproving it if you think it is incorrect.
I don't want to paper over a bug.

>> value_cast_structs only does nothing if both TYPE_NAME()s are null.
>> I was wondering if, back when the code was originally written, if
>> there never was a case when both TYPE_NAME()s were non-null--it's the
>> only way, other than simple oversight, I can account for that case
>> not being covered.

Daniel> No, it looks exactly like I intended.  If we have two classes, we
Daniel> check if A is a base class of B, then if B is a base class of A.
Daniel> If A == B, neither of those checks will be true, and we'll return
Daniel> without doing anything.

Daniel> So the problem is that one of those base class checks is calling
Daniel> error().  That's not supposed to happen.

I think from value_cast_structs:

  /* Downcasting: look in the type of the target to see if it contains the
     type of the source as a superclass.  If so, we'll need to
     offset the pointer rather than just change its type.
     FIXME: This fails silently with virtual inheritance.  */

... but this doesn't fail silently, it tries to dereference NULL, due to
the use of value_zero.  IIRC.

The reason I approved this is that it seems strange to me to try to do
any cast when we can tell beforehand that it is not needed.  It is doing
a lot of work for no reason.

Also, I tried a little to write a different test that would still hit
this failure, and I couldn't.  I don't know if Chris tried this or not.

Tom

Re: Patch for PR 9399

[Chris Moller]

On 12/10/09 14:04, Tom Tromey wrote:
>>>>>> "Daniel" == Daniel Jacobowitz<drow@false.org>  writes:
>>>>>>              
>
> Daniel>  Tom's approved this.  That's fine.
>
> I'm open to unapproving it if you think it is incorrect.
> I don't want to paper over a bug.
>
>    
>>> value_cast_structs only does nothing if both TYPE_NAME()s are null.
>>> I was wondering if, back when the code was originally written, if
>>> there never was a case when both TYPE_NAME()s were non-null--it's the
>>> only way, other than simple oversight, I can account for that case
>>> not being covered.
>>>        
>
> Daniel>  No, it looks exactly like I intended.  If we have two classes, we
> Daniel>  check if A is a base class of B, then if B is a base class of A.
> Daniel>  If A == B, neither of those checks will be true, and we'll return
> Daniel>  without doing anything.
>
> Daniel>  So the problem is that one of those base class checks is calling
> Daniel>  error().  That's not supposed to happen.
>
> I think from value_cast_structs:
>
>    /* Downcasting: look in the type of the target to see if it contains the
>       type of the source as a superclass.  If so, we'll need to
>       offset the pointer rather than just change its type.
>       FIXME: This fails silently with virtual inheritance.  */
>
> ... but this doesn't fail silently, it tries to dereference NULL, due to
> the use of value_zero.  IIRC.
>    

Yeah, that's what it does, but I've forgotten exactly where in the code 
I saw it doing that.

> The reason I approved this is that it seems strange to me to try to do
> any cast when we can tell beforehand that it is not needed.  It is doing
> a lot of work for no reason.
>
> Also, I tried a little to write a different test that would still hit
> this failure, and I couldn't.  I don't know if Chris tried this or not.
>    

Nope.

> Tom
>    

Re: Patch for PR 9399

[Daniel Jacobowitz]

No, I think the patch is fine to go in.

On Thu, Dec 10, 2009 at 12:04:09PM -0700, Tom Tromey wrote:
> I think from value_cast_structs:
> 
>   /* Downcasting: look in the type of the target to see if it contains the
>      type of the source as a superclass.  If so, we'll need to
>      offset the pointer rather than just change its type.
>      FIXME: This fails silently with virtual inheritance.  */
> 
> ... but this doesn't fail silently, it tries to dereference NULL, due to
> the use of value_zero.  IIRC.

I see now.  This is just lame.  We should be able to ask this question
about the static type of the target - possibly the easiest way to do
that is to special-case the NULL vptr.  I probably wrote that.

-- 
Daniel Jacobowitz
CodeSourcery