[RFC,PR,gdb/24154] Use real type for member functions with auto return type

Message ID 20200409152724.18581-1-ssbssa@yahoo.de
State New
Headers show
Series
  • [RFC,PR,gdb/24154] Use real type for member functions with auto return type
Related show

Commit Message

Keith Seitz via Gdb-patches April 9, 2020, 3:27 p.m.
For this code:
template<typename T>
struct X
{
  T value;
  auto get () { return value; }
};
X<int> x{1};

You get the following with gdb:
(gdb) ptype X<int>::get
type = int (X<int> * const)
(gdb) ptype x.get
type = void (X<int> * const)
(gdb) print x.get()
$1 = void

So this checks for any kind of auto return type, and uses the real type
instead.

gdb/ChangeLog:

2020-04-09  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/24154
	* dwarf2/read.c (read_subroutine_type): Change function to method.
	* value.c (value_fn_field): Use real type instead of auto.

gdb/testsuite/ChangeLog:

2020-04-09  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/24154
	* gdb.cp/auto-return.cc: New test.
	* gdb.cp/auto-return.exp: New file.
---
I'm not at all sure about my code in dwarf2/read.c, any comments welcome.
---
 gdb/dwarf2/read.c                    | 24 ++++++++++++++++++++
 gdb/testsuite/gdb.cp/auto-return.cc  | 32 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/auto-return.exp | 33 ++++++++++++++++++++++++++++
 gdb/value.c                          | 23 +++++++++++++++++++
 4 files changed, 112 insertions(+)
 create mode 100644 gdb/testsuite/gdb.cp/auto-return.cc
 create mode 100644 gdb/testsuite/gdb.cp/auto-return.exp

-- 
2.26.0

Comments

Tom Tromey April 9, 2020, 6:21 p.m. | #1
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:


Hannes> For this code:
Hannes> template<typename T>
Hannes> struct X
Hannes> {
Hannes>   T value;
Hannes>   auto get () { return value; }
Hannes> };
Hannes> X<int> x{1};

What does the resulting DWARF look like?
It would be good to have that in the commit message.

Hannes> +  /* If this function has a specification that's a class member, change
Hannes> +     it to a method  */
Hannes> +  if (cu->language == language_cplus
Hannes> +      && dwarf2_attr (die, DW_AT_specification, cu))
Hannes> +    {
Hannes> +      struct dwarf2_cu *spec_cu = cu;
Hannes> +      struct die_info *spec_die = die_specification (die, &spec_cu);

It's probably better to just call die_specification and check its
result, rather than calling dwarf2_attr.

Hannes> +	  struct type *return_type = TYPE_TARGET_TYPE (ftype);

You probably want to wrap this in check_typedef.

Hannes> +	  while (TYPE_CODE (return_type) == TYPE_CODE_REF
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_RVALUE_REF
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_PTR
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_METHOD
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_METHODPTR
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_MEMBERPTR
Hannes> +		 || TYPE_CODE (return_type) == TYPE_CODE_FUNC)
Hannes> +	    return_type = TYPE_TARGET_TYPE (return_type);

This too.

Hannes> +	  if (TYPE_CODE (return_type) == TYPE_CODE_VOID)
Hannes> +	    {
Hannes> +	      const char *name = TYPE_NAME (return_type);
Hannes> +	      if (name != nullptr
Hannes> +		  && (strcmp (name, "auto") == 0
Hannes> +		      || strcmp (name, "decltype(auto)") == 0))
Hannes> +		ftype = sym->type;

I tend to suspect we should handle this during DWARF reading.
Without seeing the DWARF I'm not sure what's really going on though.

Tom
Keith Seitz via Gdb-patches April 10, 2020, 11:24 a.m. | #2
Am Donnerstag, 9. April 2020, 20:21:51 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

>

> Hannes> For this code:

> Hannes> template<typename T>

> Hannes> struct X

> Hannes> {

> Hannes>  T value;

> Hannes>  auto get () { return value; }

> Hannes> };

> Hannes> X<int> x{1};

>

> What does the resulting DWARF look like?

> It would be good to have that in the commit message.

>

> Hannes> +  /* If this function has a specification that's a class member, change

> Hannes> +    it to a method  */

> Hannes> +  if (cu->language == language_cplus

> Hannes> +      && dwarf2_attr (die, DW_AT_specification, cu))

> Hannes> +    {

> Hannes> +      struct dwarf2_cu *spec_cu = cu;

> Hannes> +      struct die_info *spec_die = die_specification (die, &spec_cu);

>

> It's probably better to just call die_specification and check its

> result, rather than calling dwarf2_attr.

>

> Hannes> +      struct type *return_type = TYPE_TARGET_TYPE (ftype);

>

> You probably want to wrap this in check_typedef.

>

> Hannes> +      while (TYPE_CODE (return_type) == TYPE_CODE_REF

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_RVALUE_REF

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_PTR

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_METHOD

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_METHODPTR

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_MEMBERPTR

> Hannes> +        || TYPE_CODE (return_type) == TYPE_CODE_FUNC)

> Hannes> +        return_type = TYPE_TARGET_TYPE (return_type);

>

> This too.

>

> Hannes> +      if (TYPE_CODE (return_type) == TYPE_CODE_VOID)

>

> Hannes> +        {

>

> Hannes> +          const char *name = TYPE_NAME (return_type);

> Hannes> +          if (name != nullptr

> Hannes> +          && (strcmp (name, "auto") == 0

> Hannes> +              || strcmp (name, "decltype(auto)") == 0))

> Hannes> +        ftype = sym->type;

>

> I tend to suspect we should handle this during DWARF reading.

> Without seeing the DWARF I'm not sure what's really going on though.


This is all of the DWARF info:

    .uleb128 0x2     # (DIE (0x68) DW_TAG_structure_type)
    .ascii "X<int>\0"     # DW_AT_name
    .byte    0x4     # DW_AT_byte_size
    .byte    0x1     # DW_AT_decl_file (gdb-24154.cpp)
    .byte    0x2     # DW_AT_decl_line
    .byte    0x8     # DW_AT_decl_column
    .long    0xb8     # DW_AT_sibling
    .uleb128 0x3     # (DIE (0x78) DW_TAG_member)
    .ascii "value\0"     # DW_AT_name
    .byte    0x1     # DW_AT_decl_file (gdb-24154.cpp)
    .byte    0x4     # DW_AT_decl_line
    .byte    0x5     # DW_AT_decl_column
    .long    0xb8     # DW_AT_type
    .byte    0     # DW_AT_data_member_location
    .uleb128 0x4     # (DIE (0x87) DW_TAG_subprogram)
             # DW_AT_external
    .ascii "get\0"     # DW_AT_name
    .byte    0x1     # DW_AT_decl_file (gdb-24154.cpp)
    .byte    0x5     # DW_AT_decl_line
    .byte    0x8     # DW_AT_decl_column
    .ascii "_ZN1XIiE3getEv\0"     # DW_AT_linkage_name
    .long    0xbf     # DW_AT_type
             # DW_AT_declaration
    .long    0xaa     # DW_AT_object_pointer
    .long    0xb0     # DW_AT_sibling
    .uleb128 0x5     # (DIE (0xaa) DW_TAG_formal_parameter)
    .long    0xc5     # DW_AT_type
             # DW_AT_artificial
    .byte    0     # end of children of DIE 0x87
    .uleb128 0x6     # (DIE (0xb0) DW_TAG_template_type_param)
    .ascii "T\0"     # DW_AT_name
    .long    0xb8     # DW_AT_type
    .byte    0     # end of children of DIE 0x68
    .uleb128 0x7     # (DIE (0xb8) DW_TAG_base_type)
    .byte    0x4     # DW_AT_byte_size
    .byte    0x5     # DW_AT_encoding
    .ascii "int\0"     # DW_AT_name
    .uleb128 0x8     # (DIE (0xbf) DW_TAG_unspecified_type)
    .ascii "auto\0"     # DW_AT_name
    .uleb128 0x9     # (DIE (0xc5) DW_TAG_pointer_type)
    .byte    0x8     # DW_AT_byte_size
    .long    0x68     # DW_AT_type
    .uleb128 0xa     # (DIE (0xcb) DW_TAG_const_type)
    .long    0xc5     # DW_AT_type
    .uleb128 0xb     # (DIE (0xd0) DW_TAG_variable)
    .ascii "x\0"     # DW_AT_name
    .byte    0x1     # DW_AT_decl_file (gdb-24154.cpp)
    .byte    0x8     # DW_AT_decl_line
    .byte    0x8     # DW_AT_decl_column
    .long    0x68     # DW_AT_type
             # DW_AT_external
    .uleb128 0x9     # DW_AT_location
    .byte    0x3     # DW_OP_addr
    .quad    x
    .uleb128 0xc     # (DIE (0xe4) DW_TAG_subprogram)
             # DW_AT_external
    .ascii "main\0"     # DW_AT_name
    .byte    0x1     # DW_AT_decl_file (gdb-24154.cpp)
    .byte    0xb     # DW_AT_decl_line
    .byte    0x1     # DW_AT_decl_column
    .long    0xb8     # DW_AT_type
    .quad    .LFB1     # DW_AT_low_pc
    .quad    .LFE1-.LFB1     # DW_AT_high_pc
    .uleb128 0x1     # DW_AT_frame_base
    .byte    0x9c     # DW_OP_call_frame_cfa
             # DW_AT_GNU_all_tail_call_sites
    .uleb128 0xd     # (DIE (0x103) DW_TAG_subprogram)
    .long    0x87     # DW_AT_specification
    .long    0xb8     # DW_AT_type
    .long    0x122     # DW_AT_object_pointer
    .quad    .LFB2     # DW_AT_low_pc
    .quad    .LFE2-.LFB2     # DW_AT_high_pc
    .uleb128 0x1     # DW_AT_frame_base
    .byte    0x9c     # DW_OP_call_frame_cfa
             # DW_AT_GNU_all_call_sites
    .uleb128 0xe     # (DIE (0x122) DW_TAG_formal_parameter)
    .ascii "this\0"     # DW_AT_name
    .long    0xcb     # DW_AT_type
             # DW_AT_artificial
    .uleb128 0x2     # DW_AT_location
    .byte    0x91     # DW_OP_fbreg
    .sleb128 0
    .byte    0     # end of children of DIE 0x103
    .byte    0     # end of children of DIE 0xb

So the real type is in the definition die, while the class member function die
only has the auto type.


Hannes
Tom Tromey April 14, 2020, 9:34 p.m. | #3
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:


>> I tend to suspect we should handle this during DWARF reading.

>> Without seeing the DWARF I'm not sure what's really going on though.


Hannes> This is all of the DWARF info:

For when you update the commit message, IMO the output of objdump is way
more readable.

Hannes> So the real type is in the definition die, while the class
Hannes> member function die only has the auto type.

It definitely seems better to handle this in the DWARF reader then.
What do you think?

Tom
Keith Seitz via Gdb-patches April 14, 2020, 11:05 p.m. | #4
Am Mittwoch, 15. April 2020, 00:49:39 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

>

> >> I tend to suspect we should handle this during DWARF reading.

> >> Without seeing the DWARF I'm not sure what's really going on though.

>

> Hannes> This is all of the DWARF info:

>

> For when you update the commit message, IMO the output of objdump is way

> more readable.

>

> Hannes> So the real type is in the definition die, while the class

> Hannes> member function die only has the auto type.

>

> It definitely seems better to handle this in the DWARF reader then.

> What do you think?


I don't really have a strong opinion where this should be handled.

Currently gdb shows the types like this:

(gdb) pt x
type = struct X<int> [with T = int] {
    T value;
  public:
    auto get(void);
}
(gdb) pt x.get
type = void (X<int> * const)
(gdb) pt X<int>::get
type = int (X<int> * const)

And with my current changes only x.get is different:

(gdb) pt x.get
type = int (X<int> * const)

And if I understand you correctly, when you say handle in the DWARF reader,
then I think it would also change the displayed return type in the struct:

 type = struct X<int> [with T = int] {
     T value;
   public:
-    auto get(void);
+    int get(void);
 }

If this is acceptable, it's fine for me.


Hannes
Tom Tromey May 7, 2020, 9:47 p.m. | #5
Hannes> For this code:
Hannes> template<typename T>
Hannes> struct X
Hannes> {
Hannes> T value;
Hannes> auto get () { return value; }
Hannes> };
Hannes> X<int> x{1};

Tom> What does the resulting DWARF look like?
Tom> It would be good to have that in the commit message.

I compiled it myself and with gcc 9.3 I get:

 <2><37>: Abbrev Number: 4 (DW_TAG_subprogram)
    <38>   DW_AT_external    : 1
    <38>   DW_AT_name        : get
    <3c>   DW_AT_decl_file   : 1
    <3d>   DW_AT_decl_line   : 5
    <3e>   DW_AT_decl_column : 8
    <3f>   DW_AT_linkage_name: (indirect string, offset: 0xb): _ZN1XIiE3getEv
    <43>   DW_AT_type        : <0x64>
[...]
 <1><64>: Abbrev Number: 8 (DW_TAG_unspecified_type)
    <65>   DW_AT_name        : (indirect string, offset: 0x0): auto


This just seems like a compiler bug to me, and I think the return type
should either be 'int' or, ideally, the template parameter:

 <2><55>: Abbrev Number: 6 (DW_TAG_template_type_param)
    <56>   DW_AT_name        : T
    <58>   DW_AT_type        : <0x5d>

In my case there's no specification so I don't see how gdb could do
anything about this.

Tom
Keith Seitz via Gdb-patches May 8, 2020, 9:11 a.m. | #6
Am Donnerstag, 7. Mai 2020, 23:48:00 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> Hannes> For this code:

> Hannes> template<typename T>

> Hannes> struct X

> Hannes> {

> Hannes> T value;

> Hannes> auto get () { return value; }

> Hannes> };

> Hannes> X<int> x{1};

>

> Tom> What does the resulting DWARF look like?

> Tom> It would be good to have that in the commit message.

>

> I compiled it myself and with gcc 9.3 I get:

>

> <2><37>: Abbrev Number: 4 (DW_TAG_subprogram)

>     <38>  DW_AT_external    : 1

>     <38>  DW_AT_name        : get

>     <3c>  DW_AT_decl_file  : 1

>     <3d>  DW_AT_decl_line  : 5

>     <3e>  DW_AT_decl_column : 8

>     <3f>  DW_AT_linkage_name: (indirect string, offset: 0xb): _ZN1XIiE3getEv

>     <43>  DW_AT_type        : <0x64>

> [...]

> <1><64>: Abbrev Number: 8 (DW_TAG_unspecified_type)

>     <65>  DW_AT_name        : (indirect string, offset: 0x0): auto

>

>

> This just seems like a compiler bug to me, and I think the return type

> should either be 'int' or, ideally, the template parameter:

>

> <2><55>: Abbrev Number: 6 (DW_TAG_template_type_param)

>     <56>  DW_AT_name        : T

>     <58>  DW_AT_type        : <0x5d>

>

> In my case there's no specification so I don't see how gdb could do

> anything about this.


If there is no specification, then I think you're missing the actual use of
the function, like:

int
main (void)
{
  return (x.get ());
}


Hannes
Tom Tromey May 8, 2020, 8:18 p.m. | #7
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:


>> It definitely seems better to handle this in the DWARF reader then.

>> What do you think?


Hannes> I don't really have a strong opinion where this should be
Hannes> handled.

IMO it belongs in the reader, since it's working around a deficiency in
gcc's output.

Hannes> And if I understand you correctly, when you say handle in the DWARF reader,
Hannes> then I think it would also change the displayed return type in the struct:

Yes, and value.c shouldn't need a change.

Tom

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index da702205c6..1abf7ff751 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16746,6 +16746,30 @@  read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
+  /* If this function has a specification that's a class member, change
+     it to a method  */
+  if (cu->language == language_cplus
+      && dwarf2_attr (die, DW_AT_specification, cu))
+    {
+      struct dwarf2_cu *spec_cu = cu;
+      struct die_info *spec_die = die_specification (die, &spec_cu);
+
+      if (spec_die != nullptr && spec_die->parent != nullptr
+	  && spec_die->tag == DW_TAG_subprogram
+	  && (spec_die->parent->tag == DW_TAG_structure_type
+	      || spec_die->parent->tag == DW_TAG_union_type
+	      || spec_die->parent->tag == DW_TAG_class_type))
+	{
+	  struct type *self_type = read_type_die (spec_die->parent, cu);
+	  if (self_type != nullptr)
+	    smash_to_method_type (ftype, self_type,
+				  TYPE_TARGET_TYPE (ftype),
+				  TYPE_FIELDS (ftype),
+				  TYPE_NFIELDS (ftype),
+				  TYPE_VARARGS (ftype));
+	}
+    }
+
   return ftype;
 }
 
diff --git a/gdb/testsuite/gdb.cp/auto-return.cc b/gdb/testsuite/gdb.cp/auto-return.cc
new file mode 100644
index 0000000000..4f0975e196
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/auto-return.cc
@@ -0,0 +1,32 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+template<typename T>
+struct X
+{
+  T value;
+  auto get () { return value; }
+  auto &get_ref () { return value; }
+};
+
+X<int> x{1};
+
+int
+main (void)
+{
+  return (x.get () + x.get_ref ());
+}
diff --git a/gdb/testsuite/gdb.cp/auto-return.exp b/gdb/testsuite/gdb.cp/auto-return.exp
new file mode 100644
index 0000000000..7c2d78450f
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/auto-return.exp
@@ -0,0 +1,33 @@ 
+# Copyright 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/>.
+
+# 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++14}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+gdb_test "ptype x.get" "type = int \\(X<int> \\* const\\)"
+gdb_test "print x.get()" " = 1"
diff --git a/gdb/value.c b/gdb/value.c
index f722c272d8..b7b64805b1 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3048,6 +3048,29 @@  value_fn_field (struct value **arg1p, struct fn_field *f,
   if (sym != NULL)
     {
       memset (&msym, 0, sizeof (msym));
+
+      /* Check if the member function has some kind of C++14 'auto' return
+	 type, and use the real type in this case instead.  */
+      if (TYPE_CODE (ftype) == TYPE_CODE_METHOD)
+	{
+	  struct type *return_type = TYPE_TARGET_TYPE (ftype);
+	  while (TYPE_CODE (return_type) == TYPE_CODE_REF
+		 || TYPE_CODE (return_type) == TYPE_CODE_RVALUE_REF
+		 || TYPE_CODE (return_type) == TYPE_CODE_PTR
+		 || TYPE_CODE (return_type) == TYPE_CODE_METHOD
+		 || TYPE_CODE (return_type) == TYPE_CODE_METHODPTR
+		 || TYPE_CODE (return_type) == TYPE_CODE_MEMBERPTR
+		 || TYPE_CODE (return_type) == TYPE_CODE_FUNC)
+	    return_type = TYPE_TARGET_TYPE (return_type);
+	  if (TYPE_CODE (return_type) == TYPE_CODE_VOID)
+	    {
+	      const char *name = TYPE_NAME (return_type);
+	      if (name != nullptr
+		  && (strcmp (name, "auto") == 0
+		      || strcmp (name, "decltype(auto)") == 0))
+		ftype = sym->type;
+	    }
+	}
     }
   else
     {