[PATCH] Fix crash in cp_print_value_fields

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix crash in cp_print_value_fields

Tom Tromey-4
From: Tom Tromey <[hidden email]>

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  <[hidden email]>

        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  <[hidden email]>

        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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix crash in cp_print_value_fields

Tom Tromey-4
Tom> 2019-05-17  Tom Tromey  <[hidden email]>

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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix crash in cp_print_value_fields

Pedro Alves-7
In reply to this post by Tom Tromey-4
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix crash in cp_print_value_fields

Tom de Vries
In reply to this post by Tom Tromey-4
On 17-05-19 22:18, Tom Tromey wrote:

> From: Tom Tromey <[hidden email]>
>
> 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  <[hidden email]>
>
> 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  <[hidden email]>
>
> 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)
>   {
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix crash in cp_print_value_fields

Tom Tromey-2
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix crash in cp_print_value_fields

Tom Tromey-2
In reply to this post by Tom de Vries
>>>>> "Tom" == Tom de Vries <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

[8.3 backport] Fix crash in cp_print_value_fields

Tom de Vries
In reply to this post by Tom Tromey-4
On 17-05-19 22:18, Tom Tromey wrote:

> From: Tom Tromey <[hidden email]>
>
> 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  <[hidden email]>
>
> 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  <[hidden email]>
>
> 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

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  <[hidden email]>

        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  <[hidden email]>

        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  <[hidden email]>
+
+ PR c++/20020:
+ * cp-valprint.c (cp_print_value_fields): Call
+ cp_print_static_field inside "try".
+
 2019-06-10  Tom de Vries  <[hidden email]>
 
  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  <[hidden email]>
+
+ PR c++/20020:
+ * gdb.cp/constexpr-field.exp: New file.
+ * gdb.cp/constexpr-field.cc: New file.
+
 2019-04-30  Tom Tromey  <[hidden email]>
 
  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}"

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  <[hidden email]>

        * 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  <[hidden email]>
+
+ * gdb.cp/constexpr-field.exp: Use setup_xfail.
+
 2019-05-29  Tom Tromey  <[hidden email]>
 
  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}"
Reply | Threaded
Open this post in threaded view
|

Re: [8.3 backport] Fix crash in cp_print_value_fields

Tom Tromey-4
>>>>> "Tom" == Tom de Vries <[hidden email]> 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