[PATCH] Fix crash in cp_print_value_fields

[PATCH] Fix crash in cp_print_value_fields

[Tom Tromey]

From: Tom Tromey <tom@tromey.com>

PR c++/20020 concerns a crash in cp_print_value_fields.  The immediate
cause is that cp_print_value_fields does not handle the case where
value_static_field fails.  This is fixed in this patch by calling
cp_print_static_field from the "try" block.

Digging a bit deeper, the error occurs because GCC does not emit a
DW_AT_const_value for a static constexpr member appearing in a
template class.  I've filed a GCC bug for this.

Tested on x86-64 Fedora 29.

gdb/ChangeLog
2019-05-17  Tom Tromey  <tromey@adacore.com>

	PR c++/20020:
	* cp-valprint.c (cp_print_value_fields): Call
	cp_print_static_field inside "try".

gdb/testsuite/ChangeLog
2019-05-17  Tom Tromey  <tromey@adacore.com>

	PR c++/20020:
	* gdb.cp/constexpr-field.exp: New file.
	* gdb.cp/constexpr-field.cc: New file.
---
 gdb/ChangeLog                            |  6 ++++
 gdb/cp-valprint.c                        | 12 ++++----
 gdb/testsuite/ChangeLog                  |  6 ++++
 gdb/testsuite/gdb.cp/constexpr-field.cc  | 37 ++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/constexpr-field.exp | 36 +++++++++++++++++++++++
 5 files changed, 90 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/constexpr-field.cc
 create mode 100644 gdb/testsuite/gdb.cp/constexpr-field.exp

diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
index ff860df499a..d8d5c06fd1e 100644
--- a/gdb/cp-valprint.c
+++ b/gdb/cp-valprint.c
@@ -329,22 +329,20 @@ cp_print_value_fields (struct type *type, struct type *real_type,
 		}
 	      else if (field_is_static (&TYPE_FIELD (type, i)))
 		{
-		  struct value *v = NULL;
-
 		  try
 		    {
-		      v = value_static_field (type, i);
-		    }
+		      struct value *v = value_static_field (type, i);
 
+		      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
+					     v, stream, recurse + 1,
+					     options);
+		    }
 		  catch (const gdb_exception_error &ex)
 		    {
 		      fprintf_filtered (stream,
 					_("<error reading variable: %s>"),
 					ex.what ());
 		    }
-
-		  cp_print_static_field (TYPE_FIELD_TYPE (type, i),
-					 v, stream, recurse + 1, opts);
 		}
 	      else if (i == vptr_fieldno && type == vptr_basetype)
 		{
diff --git a/gdb/testsuite/gdb.cp/constexpr-field.cc b/gdb/testsuite/gdb.cp/constexpr-field.cc
new file mode 100644
index 00000000000..ac2e514cb38
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/constexpr-field.cc
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 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/>.  */
+
+template<typename T>
+struct X
+{
+  static constexpr bool f = true;
+};
+
+X<int> x;
+
+struct Y
+{
+  static constexpr bool f = true;
+};
+
+Y y;
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/constexpr-field.exp b/gdb/testsuite/gdb.cp/constexpr-field.exp
new file mode 100644
index 00000000000..e4849efeae9
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/constexpr-field.exp
@@ -0,0 +1,36 @@
+# Copyright 2019 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 is part of the gdb testsuite.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 {c++ debug additional_flags=-std=c++17}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# "x" sometimes isn't available due to
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90526
+gdb_test "print x" " = {static f = .*}"
+
+gdb_test "print y" " = {static f = true}"
-- 
2.20.1

Re: [PATCH] Fix crash in cp_print_value_fields

[Tom Tromey]

Tom> 2019-05-17  Tom Tromey  <tromey@adacore.com>

Tom> 	PR c++/20020:
Tom> 	* cp-valprint.c (cp_print_value_fields): Call
Tom> 	cp_print_static_field inside "try".

I'm checking this in now.

Tom

Re: [PATCH] Fix crash in cp_print_value_fields

[Pedro Alves]

On 5/17/19 9:18 PM, Tom Tromey wrote:
> +# "x" sometimes isn't available due to
> +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90526
> +gdb_test "print x" " = {static f = .*}"
> +

How about making that expect the correct value, and then use
"setup_xfail gcc/90526" ?

Thanks,
Pedro Alves

Re: [PATCH] Fix crash in cp_print_value_fields

[Tom de Vries]

On 17-05-19 22:18, Tom Tromey wrote:
> From: Tom Tromey <tom@tromey.com>
> 
> PR c++/20020 concerns a crash in cp_print_value_fields.  The immediate
> cause is that cp_print_value_fields does not handle the case where
> value_static_field fails.  This is fixed in this patch by calling
> cp_print_static_field from the "try" block.
> 
> Digging a bit deeper, the error occurs because GCC does not emit a
> DW_AT_const_value for a static constexpr member appearing in a
> template class.  I've filed a GCC bug for this.
> 
> Tested on x86-64 Fedora 29.
> 
> gdb/ChangeLog
> 2019-05-17  Tom Tromey  <tromey@adacore.com>
> 
> 	PR c++/20020:
> 	* cp-valprint.c (cp_print_value_fields): Call
> 	cp_print_static_field inside "try".
> 
> gdb/testsuite/ChangeLog
> 2019-05-17  Tom Tromey  <tromey@adacore.com>
> 
> 	PR c++/20020:
> 	* gdb.cp/constexpr-field.exp: New file.
> 	* gdb.cp/constexpr-field.cc: New file.
> ---
>  gdb/ChangeLog                            |  6 ++++
>  gdb/cp-valprint.c                        | 12 ++++----
>  gdb/testsuite/ChangeLog                  |  6 ++++
>  gdb/testsuite/gdb.cp/constexpr-field.cc  | 37 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.cp/constexpr-field.exp | 36 +++++++++++++++++++++++
>  5 files changed, 90 insertions(+), 7 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.cp/constexpr-field.cc
>  create mode 100644 gdb/testsuite/gdb.cp/constexpr-field.exp
> 
> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
> index ff860df499a..d8d5c06fd1e 100644
> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> @@ -329,22 +329,20 @@ cp_print_value_fields (struct type *type, struct type *real_type,
>  		}
>  	      else if (field_is_static (&TYPE_FIELD (type, i)))
>  		{
> -		  struct value *v = NULL;
> -
>  		  try
>  		    {
> -		      v = value_static_field (type, i);
> -		    }
> +		      struct value *v = value_static_field (type, i);
>  
> +		      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> +					     v, stream, recurse + 1,
> +					     options);
> +		    }

Here options is used ...

>  		  catch (const gdb_exception_error &ex)
>  		    {
>  		      fprintf_filtered (stream,
>  					_("<error reading variable: %s>"),
>  					ex.what ());
>  		    }
> -
> -		  cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> -					 v, stream, recurse + 1, opts);

while here opts used to be used.

Is this change intentional? It's not mentioned anywhere.

Thanks,
- Tom

>  		}
>  	      else if (i == vptr_fieldno && type == vptr_basetype)
>  		{

Re: [PATCH] Fix crash in cp_print_value_fields

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 5/17/19 9:18 PM, Tom Tromey wrote:
>> +# "x" sometimes isn't available due to
>> +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90526
>> +gdb_test "print x" " = {static f = .*}"
>> +

Pedro> How about making that expect the correct value, and then use
Pedro> "setup_xfail gcc/90526" ?

Sorry about the delay on this.  I made this change, and I'll send it soon.

Tom

Re: [PATCH] Fix crash in cp_print_value_fields

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

>> +		      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
>> +					     v, stream, recurse + 1,
>> +					     options);
>> +		    }

Tom> Here options is used ...

>> catch (const gdb_exception_error &ex)
>> {
>> fprintf_filtered (stream,
>> _("<error reading variable: %s>"),
>> ex.what ());
>> }
>> -
>> -		  cp_print_static_field (TYPE_FIELD_TYPE (type, i),
>> -					 v, stream, recurse + 1, opts);

Tom> while here opts used to be used.

Tom> Is this change intentional? It's not mentioned anywhere.

Thanks for catching that.  It was not intentional.  I'll send a fix shortly.

Tom

[8.3 backport] Fix crash in cp_print_value_fields

[Tom de Vries]

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

On 17-05-19 22:18, Tom Tromey wrote:
> From: Tom Tromey <tom@tromey.com>
> 
> PR c++/20020 concerns a crash in cp_print_value_fields.  The immediate
> cause is that cp_print_value_fields does not handle the case where
> value_static_field fails.  This is fixed in this patch by calling
> cp_print_static_field from the "try" block.
> 
> Digging a bit deeper, the error occurs because GCC does not emit a
> DW_AT_const_value for a static constexpr member appearing in a
> template class.  I've filed a GCC bug for this.
> 
> Tested on x86-64 Fedora 29.
> 
> gdb/ChangeLog
> 2019-05-17  Tom Tromey  <tromey@adacore.com>
> 
> 	PR c++/20020:
> 	* cp-valprint.c (cp_print_value_fields): Call
> 	cp_print_static_field inside "try".
> 
> gdb/testsuite/ChangeLog
> 2019-05-17  Tom Tromey  <tromey@adacore.com>
> 
> 	PR c++/20020:
> 	* gdb.cp/constexpr-field.exp: New file.
> 	* gdb.cp/constexpr-field.cc: New file.

OK to backport to 8.3, as well as the followup patch "Fix two buglets in
cp_print_value_fields patch"?

Neither of the patches apply cleanly, so I'm attaching them here.

The difference in test results is:
...
+Running gdb/testsuite/gdb.cp/constexpr-field.exp ...
+XFAIL: gdb.cp/constexpr-field.exp: print x (PRMS gcc/90526)
+PASS: gdb.cp/constexpr-field.exp: print y
...

Thanks,
- Tom

[-- Attachment #2: 0001-Fix-crash-in-cp_print_value_fields.patch --]
[-- Type: text/x-patch, Size: 5553 bytes --]

Fix crash in cp_print_value_fields

[ Backport of master commit 4330d61dfb. ]

PR c++/20020 concerns a crash in cp_print_value_fields.  The immediate
cause is that cp_print_value_fields does not handle the case where
value_static_field fails.  This is fixed in this patch by calling
cp_print_static_field from the "try" block.

Digging a bit deeper, the error occurs because GCC does not emit a
DW_AT_const_value for a static constexpr member appearing in a
template class.  I've filed a GCC bug for this.

Tested on x86-64 Fedora 29.

gdb/ChangeLog
2019-05-29  Tom Tromey  <tromey@adacore.com>

	PR c++/20020:
	* cp-valprint.c (cp_print_value_fields): Call
	cp_print_static_field inside "try".

gdb/testsuite/ChangeLog
2019-05-29  Tom Tromey  <tromey@adacore.com>

	PR c++/20020:
	* gdb.cp/constexpr-field.exp: New file.
	* gdb.cp/constexpr-field.cc: New file.

---
 gdb/ChangeLog                            |  6 ++++++
 gdb/cp-valprint.c                        | 13 +++++------
 gdb/testsuite/ChangeLog                  |  6 ++++++
 gdb/testsuite/gdb.cp/constexpr-field.cc  | 37 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/constexpr-field.exp | 36 +++++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ee958c3f6e..f15cc5fcf0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-05-29  Tom Tromey  <tromey@adacore.com>
+
+	PR c++/20020:
+	* cp-valprint.c (cp_print_value_fields): Call
+	cp_print_static_field inside "try".
+
 2019-06-10  Tom de Vries  <tdevries@suse.de>
 
 	PR symtab/24545
diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
index 7124da42ec..8feaae03aa 100644
--- a/gdb/cp-valprint.c
+++ b/gdb/cp-valprint.c
@@ -314,13 +314,14 @@ cp_print_value_fields (struct type *type, struct type *real_type,
 		}
 	      else if (field_is_static (&TYPE_FIELD (type, i)))
 		{
-		  struct value *v = NULL;
-
 		  TRY
 		    {
-		      v = value_static_field (type, i);
-		    }
+		      struct value *v = value_static_field (type, i);
 
+		      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
+					     v, stream, recurse + 1,
+					     options);
+		    }
 		  CATCH (ex, RETURN_MASK_ERROR)
 		    {
 		      fprintf_filtered (stream,
@@ -328,10 +329,6 @@ cp_print_value_fields (struct type *type, struct type *real_type,
 					ex.message);
 		    }
 		  END_CATCH
-
-		  cp_print_static_field (TYPE_FIELD_TYPE (type, i),
-					 v, stream, recurse + 1,
-					 options);
 		}
 	      else if (i == vptr_fieldno && type == vptr_basetype)
 		{
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index ea1f78cc4b..5f26daa67e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2019-05-29  Tom Tromey  <tromey@adacore.com>
+
+	PR c++/20020:
+	* gdb.cp/constexpr-field.exp: New file.
+	* gdb.cp/constexpr-field.cc: New file.
+
 2019-04-30  Tom Tromey  <tromey@adacore.com>
 
 	PR c++/24470:
diff --git a/gdb/testsuite/gdb.cp/constexpr-field.cc b/gdb/testsuite/gdb.cp/constexpr-field.cc
new file mode 100644
index 0000000000..ac2e514cb3
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/constexpr-field.cc
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 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/>.  */
+
+template<typename T>
+struct X
+{
+  static constexpr bool f = true;
+};
+
+X<int> x;
+
+struct Y
+{
+  static constexpr bool f = true;
+};
+
+Y y;
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/constexpr-field.exp b/gdb/testsuite/gdb.cp/constexpr-field.exp
new file mode 100644
index 0000000000..e4849efeae
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/constexpr-field.exp
@@ -0,0 +1,36 @@
+# Copyright 2019 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 is part of the gdb testsuite.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 {c++ debug additional_flags=-std=c++17}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# "x" sometimes isn't available due to
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90526
+gdb_test "print x" " = {static f = .*}"
+
+gdb_test "print y" " = {static f = true}"

[-- Attachment #3: 0002-Fix-buglet-in-cp_print_value_fields-patch.patch --]
[-- Type: text/x-patch, Size: 1374 bytes --]

Fix buglet in cp_print_value_fields patch

[ Backport of master commit 3d507ff23b. ]

Pedro pointed out an issue in the cp_print_value_fields
patch, aka the fix for PR c++/20020.

This patch addresses the issue.  Tested on x86-64 Fedora 29.

gdb/testsuite/ChangeLog
2019-06-27  Tom Tromey  <tromey@adacore.com>

	* gdb.cp/constexpr-field.exp: Use setup_xfail.

---
 gdb/testsuite/ChangeLog                  | 4 ++++
 gdb/testsuite/gdb.cp/constexpr-field.exp | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 5f26daa67e..c51036165c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2019-06-27  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.cp/constexpr-field.exp: Use setup_xfail.
+
 2019-05-29  Tom Tromey  <tromey@adacore.com>
 
 	PR c++/20020:
diff --git a/gdb/testsuite/gdb.cp/constexpr-field.exp b/gdb/testsuite/gdb.cp/constexpr-field.exp
index e4849efeae..2f71cef952 100644
--- a/gdb/testsuite/gdb.cp/constexpr-field.exp
+++ b/gdb/testsuite/gdb.cp/constexpr-field.exp
@@ -31,6 +31,7 @@ if {![runto_main]} {
 
 # "x" sometimes isn't available due to
 # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90526
-gdb_test "print x" " = {static f = .*}"
+setup_xfail *-*-* gcc/90526
+gdb_test "print x" " = {static f = true}"
 
 gdb_test "print y" " = {static f = true}"

Re: [8.3 backport] Fix crash in cp_print_value_fields

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> OK to backport to 8.3, as well as the followup patch "Fix two buglets in
Tom> cp_print_value_fields patch"?

Yes, thank you for doing this.

Tom