Move "register" test out of classes.exp to a separate testcase (Re: [PATCH] gdb/testsuite: ISO C++17 does not allow register storage class specifier.)

Message ID 4a45bd6b-2d4c-10c9-03af-2a8be5e37783@palves.net
State New
Headers show
Series
  • Move "register" test out of classes.exp to a separate testcase (Re: [PATCH] gdb/testsuite: ISO C++17 does not allow register storage class specifier.)
Related show

Commit Message

Pedro Alves Aug. 22, 2020, 5:08 p.m.
On 8/22/20 3:54 PM, Simon Marchi wrote:
>> --- a/gdb/testsuite/gdb.cp/classes.exp

>> +++ b/gdb/testsuite/gdb.cp/classes.exp

>> @@ -25,7 +25,7 @@ load_lib "cp-support.exp"

>>  standard_testfile .cc

>>  

>>  if {[prepare_for_testing "failed to prepare" $testfile $srcfile \

>> -	 {debug c++ additional_flags=-Wno-deprecated-register}]} {

>> +	{debug c++ additional_flags=-Wno-deprecated-register additional_flags=-Wno-register}]} {

>>      return -1

>>  }

> The register keyword in this test is used here:

> 

>   629   /* We don't call any methods for v, so gcc version cygnus-2.3.3-930220

>   630      might put this variable in a register.  This is a lose, though, because

>   631      it means that GDB can't call any methods for that variable.  */

>   632   register small v;

> 

> Can we first check if we can simply remove the register keyword from the test?

> 

> I don't understand the comment above: do we want v to be in a register or not?  It sounds

> like we don't, we want it to be in memory so we can call a method on it, from GDB.  So why

> use the register keyword?

> 


The comment seems like something that would have been written before
the testcase was extended to actually test the behavior when a variable
is put in a register.

See patch below.  I propose we push this in instead.

From f984e6ec2047bc54c79cf32b5cfb0613b85729b8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Sat, 22 Aug 2020 17:39:04 +0100
Subject: [PATCH] Move "register" test out of classes.exp to a separate
 testcase

The gdb.cp/classes.exp testcase has one test that tries to exercise
the case of calling a method on a variable that has been put in a
register.

See the declaration of small in classes.cc:

/* Try to get the compiler to allocate a class in a register.  */
class small {
 public:
  int x;
  int method ();
};

and the comment in classes.exp:

    # This class is so small that an instance of it can fit in a register.
    # When gdb tries to call a method, it gets embarrassed about taking
    # the address of a register.
    #
    # TODO: I think that message should be a PASS, not an XFAIL.
    # gdb prints an informative message and declines to do something
    # impossible.
    #
    # The method call actually succeeds if the compiler allocates very
    # small classes in memory instead of registers.  So this test does
    # not tell us anything interesting if the call succeeds.
    #
    # -- chastain 2003-12-31

And these comments:

 https://gcc.gnu.org/legacy-ml/gcc/2010-05/msg00116.html
 https://gcc.gnu.org/legacy-ml/gcc/2010-05/msg00117.html

 "register keyword has other uses, e.g. for -O0 code variables
 declared with register keyword can be put into registers, while
 variables declared without it always get stack slots."

 "I think it does, without optimization.  There's some unique GDB
 tests that use this.  It causes them to be live between statements in
 a machine register instead of always stored in stack slots."

The "register" keyword seems to be ignored by the compiler nowadays
even at -O0, though.  With or without the register keyword, the
variable is given a stack slot, at least on x86-64 with GCC 9.

However, if we use the GCC extension to put the variable
in a specific variable:

 https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Local-Register-Variables.html#Local-Register-Variables

 diff --git c/gdb/testsuite/gdb.cp/classes.cc w/gdb/testsuite/gdb.cp/classes.cc
 index 5ea360e4d06..6dcf34689b8 100644
 --- c/gdb/testsuite/gdb.cp/classes.cc
 +++ w/gdb/testsuite/gdb.cp/classes.cc
 @@ -629,7 +629,7 @@ register_class ()
    /* We don't call any methods for v, so gcc version cygnus-2.3.3-930220
       might put this variable in a register.  This is a lose, though, because
       it means that GDB can't call any methods for that variable.  */
 -  register small v;
 +  register small v asm ("rax");

then it works, and we get an XFAIL:

 print v.method ()
 Address requested for identifier "v" which is in register $rax
 (gdb) XFAIL: gdb.cp/classes.exp: calling method for small class (PRMS 2972)

I think that what we should do here is move this test into its own
file, use that GCC syntax to force it to a register, and do as the
comment says -- issue a pass instead of an XFAIL.

That's what this commit does.

Note that we don't need -Wno-deprecated-register (nor -Wno-register)
anymore in the new testcase, because GNU register-asm local variables
don't trigger the warning, with either GCC or Clang.

gdb/testsuite/ChangeLog:

	* gdb.cp/classes.exp: No longer pass -Wno-deprecated-register.
	(do_tests): Remove "calling method for small class" test.
	* gdb.cp/classes.cc (class small, small::method, marker_reg1)
	(register_class): Delete.
	(main): Don't call register_class.
	* gdb.cp/call-method-register.exp: New file, based on bits removed
	from classes.exp.
	* gdb.cp/call-method-register.cc: New file, based on bits removed
	from classes.cc.
---
 gdb/testsuite/gdb.cp/call-method-register.cc  | 66 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/call-method-register.exp | 57 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/classes.cc               | 37 ---------------
 gdb/testsuite/gdb.cp/classes.exp              | 36 +--------------
 4 files changed, 124 insertions(+), 72 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/call-method-register.cc
 create mode 100644 gdb/testsuite/gdb.cp/call-method-register.exp


base-commit: 33bf4c5c10d246543c7a9a8ba4bbbe173d7f3367
-- 
2.14.5

Patch

diff --git a/gdb/testsuite/gdb.cp/call-method-register.cc b/gdb/testsuite/gdb.cp/call-method-register.cc
new file mode 100644
index 00000000000..2beed47c13d
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/call-method-register.cc
@@ -0,0 +1,66 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 1993-2020 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/>.  */
+
+#if defined __x86_64__
+# define ASM_REG "rax"
+#elif defined __i386__
+# define ASM_REG "eax"
+#else
+# error "port me"
+#endif
+
+/* A class small enough that it fits in a register.  */
+struct small
+{
+  int x;
+  int method ();
+};
+
+int
+small::method ()
+{
+  return x + 5;
+}
+
+int
+register_class ()
+{
+  /* Given the use of the GNU register-asm local variables extension,
+     the compiler puts this variable in a register.  This means that
+     GDB can't call any methods for this variable, which is what we
+     want to test.  */
+  register small v asm (ASM_REG);
+
+  int i;
+
+  /* Perform a computation sufficiently complicated that optimizing
+     compilers won't optimize out the variable.  If some compiler
+     constant-folds this whole loop, maybe using a parameter to this
+     function here would help.  */
+  v.x = 0;
+  for (i = 0; i < 13; ++i)
+    v.x += i;
+  --v.x; /* v.x is now 77 */
+  return v.x + 5; /* set breakpoint here */
+}
+
+int
+main ()
+{
+  register_class ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/call-method-register.exp b/gdb/testsuite/gdb.cp/call-method-register.exp
new file mode 100644
index 00000000000..d12f0ef86b9
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/call-method-register.exp
@@ -0,0 +1,57 @@ 
+# Copyright 1992-2020 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/>.
+
+# Test callling a method on a variable that has been put in a
+# register.
+
+if { [skip_cplus_tests] } { continue }
+
+load_lib "cp-support.exp"
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+proc test_call_register_class {} {
+    global gdb_prompt
+
+    if ![runto_main] {
+	fail "couldn't run to main"
+	return
+    }
+
+    set bp_location [gdb_get_line_number "set breakpoint here"]
+    gdb_breakpoint $bp_location
+    gdb_continue_to_breakpoint "break here"
+
+    # This class is so small that an instance of it can fit in a register.
+    # When gdb tries to call a method, it gets embarrassed about taking
+    # the address of a register.
+    #
+    # That message is a PASS, not an XFAIL, because gdb prints an
+    # informative message and declines to do something impossible.
+    #
+    # The method call actually succeeds if the compiler allocates very
+    # small classes in memory instead of registers.  If that happens,
+    # it's a FAIL, because the testcase is written in a form such that
+    # it should not happen.
+    gdb_test "print v.method ()" \
+	"Address requested for identifier \"v\" which is in register .*" \
+	"call method on register local"
+}
+
+test_call_register_class
diff --git a/gdb/testsuite/gdb.cp/classes.cc b/gdb/testsuite/gdb.cp/classes.cc
index 5ea360e4d06..e8bac54fc17 100644
--- a/gdb/testsuite/gdb.cp/classes.cc
+++ b/gdb/testsuite/gdb.cp/classes.cc
@@ -532,19 +532,6 @@  typedef struct {
 } tagless_struct;
 tagless_struct v_tagless;
 
-/* Try to get the compiler to allocate a class in a register.  */
-class small {
- public:
-  int x;
-  int method ();
-};
-
-int
-small::method ()
-{
-  return x + 5;
-}
-
 class class_with_typedefs
 {
 public:
@@ -621,29 +608,6 @@  private:
   INT b;
 };
 
-void marker_reg1 () {}
-
-int
-register_class ()
-{
-  /* We don't call any methods for v, so gcc version cygnus-2.3.3-930220
-     might put this variable in a register.  This is a lose, though, because
-     it means that GDB can't call any methods for that variable.  */
-  register small v;
-
-  int i;
-
-  /* Perform a computation sufficiently complicated that optimizing compilers
-     won't optimized out the variable.  If some compiler constant-folds this
-     whole loop, maybe using a parameter to this function here would help.  */
-  v.x = 0;
-  for (i = 0; i < 13; ++i)
-    v.x += i;
-  --v.x; /* v.x is now 77 */
-  marker_reg1 ();
-  return v.x + 5;
-}
-
 void dummy()
 {
   v_bool = true;
@@ -686,7 +650,6 @@  main()
   inheritance1 ();
   inheritance3 ();
   enums1 ();
-  register_class ();
 
   /* FIXME: pmi gets optimized out.  Need to do some more computation with
      it or something.  (No one notices, because the test is xfail'd anyway,
diff --git a/gdb/testsuite/gdb.cp/classes.exp b/gdb/testsuite/gdb.cp/classes.exp
index 4a2287a8704..ae4bf13bb50 100644
--- a/gdb/testsuite/gdb.cp/classes.exp
+++ b/gdb/testsuite/gdb.cp/classes.exp
@@ -24,8 +24,7 @@  load_lib "cp-support.exp"
 
 standard_testfile .cc
 
-if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
-	 {debug c++ additional_flags=-Wno-deprecated-register}]} {
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
     return -1
 }
 
@@ -650,39 +649,6 @@  proc do_tests {} {
 
     # Now some random tests that were just thrown in here.
 
-    gdb_breakpoint marker_reg1
-    gdb_test "continue" ".*Breakpoint .* marker_reg1.*" ""
-    gdb_test "finish" "Run till exit from.*" "finish from marker_reg1"
-
-    # This class is so small that an instance of it can fit in a register.
-    # When gdb tries to call a method, it gets embarrassed about taking
-    # the address of a register.
-    #
-    # TODO: I think that message should be a PASS, not an XFAIL.
-    # gdb prints an informative message and declines to do something
-    # impossible.
-    #
-    # The method call actually succeeds if the compiler allocates very
-    # small classes in memory instead of registers.  So this test does
-    # not tell us anything interesting if the call succeeds.
-    #
-    # -- chastain 2003-12-31
-    gdb_test_multiple "print v.method ()" "calling method for small class" {
-	-re "\\$\[0-9\]+ = 82$nl$gdb_prompt $" {
-	    # gcc 3.3.2 -gdwarf-2
-	    # gcc HEAD 2003-12-28 21:08:30 UTC -gdwarf-2
-	    # gcc 3.3.2 -gstabs+
-	    # gcc HEAD 2003-12-28 21:08:30 UTC -gstabs+
-	    pass "calling method for small class"
-	}
-	-re "Address requested for identifier \"v\" which is in register .*$nl$gdb_prompt $" {
-	    # gcc 2.95.3 -gdwarf-2
-	    # gcc 2.95.3 -gstabs+
-	    setup_xfail "*-*-*" 2972
-	    fail "calling method for small class"
-	}
-    }
-
     gdb_test "print base1::Base1" "<.*Base1.*>" "print ctor of typedef class"
     gdb_test "print base1::~Base1" "<.*~Base1(\\(\\))?>" \
 	"print dtor of typedef class"