diff mbox

[review] gdb: recognize new DWARF attributes: defaulted, deleted, calling conv.

Message ID gerrit.1571406803000.I54192f363115b78ec7435a8563b73fcace420765@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 18, 2019, 1:53 p.m. UTC 
  Tankut Baris Aktemur has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................

gdb: recognize new DWARF attributes: defaulted, deleted, calling conv.

Extend GDB's internal representation of types to include the
DW_AT_calling_convention, DW_AT_defaulted, and DW_AT_deleted attributes
that were introduced in DWARF5.

gdb/ChangeLog:
2019-MM-DD  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* dwarf2read.c (dwarf2_add_member_fn): Read the DW_AT_defaulted
	and DW_AT_deleted attributes of a function.
	(read_structure_type): Read the	DW_AT_calling_convention attribute
	of a type.
	* gdbtypes.h (struct fn_field)<defaulted>: New field to store the
	DW_AT_defaulted attribute.
	(struct fn_field)<is_deleted>: New field to store the DW_AT_deleted
	attribute.
	(struct cplus_struct_type)<calling_convention>: New field to store
	the DW_AT_calling_convention attribute.
	(TYPE_FN_FIELD_DEFAULTED): New macro.
	(TYPE_FN_FIELD_DELETED): New macro.
	(TYPE_CPLUS_CALLING_CONVENTION): New macro.
	* gdbtypes.c:  (dump_fn_fieldlists): Update for the changes made
	to the .h file.
	(print_cplus_stuff): Likewise.

Change-Id: I54192f363115b78ec7435a8563b73fcace420765
---
M gdb/dwarf2read.c
M gdb/gdbtypes.c
M gdb/gdbtypes.h
3 files changed, 47 insertions(+), 1 deletion(-)

Comments

Simon Marchi (Code Review) Oct. 18, 2019, 2:02 p.m. UTC | #1 
Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

v2 of this patch: https://sourceware.org/ml/gdb-patches/2019-04/msg00474.html
with your comments addressed.
Simon Marchi (Code Review) Oct. 23, 2019, 2:31 p.m. UTC | #2 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(2 comments)

Thank you for the patch.  This looks essentially good to me.  I had a couple of nits,
related to structure packing and size.

I assume this is used elsewhere in your series.  Now that we're on gerrit and can't
do cover letters, I think it would be helpful if supporting patches like this could
mention facts like that... something for us all to consider as we write commit
messages.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@929 
PS1, Line 929:   unsigned int defaulted;
Should this be a bit-field or a narrower integer type?
I see `DW_DEFAULTED` only has 3 values at present.

I guess we'd have to deal with the situation where the DWARF
gives some random value here?

Offhand I'm not sure whether space is a big consideration here
or not, but my default with symbols and types is to assume that it is.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101:     unsigned calling_convention : 8;
This might pack better up near the `is_dynamic` field.
Simon Marchi (Code Review) Oct. 29, 2019, 10:20 p.m. UTC | #3 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101: 
 986 | struct cplus_struct_type
     | ...
1092 |        N_TEMPLATE_ARGUMENTS elements.  This is NULL for non-template
1093 |        classes.  */
1094 | 
1095 |     struct symbol **template_arguments;
1096 | 
1097 |     /* * The calling convention for this type, fetched from the
1098 |        DW_AT_calling_convention attribute.  The value is one of the
1099 |        DW_CC enum dwarf_calling_convention constants.  */
1100 | 
1101 |     unsigned calling_convention : 8;

> Same here, I would suggest having our own enum, having the dwarf parser validate the input value and convert it to our enum type.

FWIW I thought we had one other case where we just reused a
DWARF enum in gdbtypes?  It seemed to me that this was ok,
because (1) DWARF is probably a superset of anything else we're
likely to deal with, and (2) if not and something else comes along,
we can always require a new enum at that point.

What do you think of this approach?
Simon Marchi (Code Review) Oct. 30, 2019, 3:30 a.m. UTC | #4 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101: 
 986 | struct cplus_struct_type
     | ...
1092 |        N_TEMPLATE_ARGUMENTS elements.  This is NULL for non-template
1093 |        classes.  */
1094 | 
1095 |     struct symbol **template_arguments;
1096 | 
1097 |     /* * The calling convention for this type, fetched from the
1098 |        DW_AT_calling_convention attribute.  The value is one of the
1099 |        DW_CC enum dwarf_calling_convention constants.  */
1100 | 
1101 |     unsigned calling_convention : 8;

> > Same here, I would suggest having our own enum, having the dwarf parser validate the input value a […]

That makes sense, this is not API so we can change it however we want.  So it's a good argument for not over-complicating things for nothing.

I still think that this field should have the right enum type (enum dwarf_calling_convention), I don't see any downsides to it.
Simon Marchi (Code Review) Oct. 31, 2019, 9:43 a.m. UTC | #5 
Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(2 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101: 
 986 | struct cplus_struct_type
     | ...
1096 | 
1097 |     /* * The calling convention for this type, fetched from the
1098 |        DW_AT_calling_convention attribute.  The value is one of the
1099 |        DW_CC enum dwarf_calling_convention constants.  */
1100 | 
1101 >     unsigned calling_convention : 8;
1102 |   };
1103 | 
1104 | /* * Struct used to store conversion rankings.  */
1105 | 
1106 | struct rank

> That makes sense, this is not API so we can change it however we want. […]

I assume this comment applies to the "defaulted" attribute, too.  Before I send a revision, just to clarify: I don't see another use of "enum dwarf_*" in gdbtypes.h.  In the original patch, my aim was to be consistent with the existing definition; in particular with the 'calling_convention' field in 'func_type' a little bit down (marked with another comment).  So, my question is, would this change create inconsistency in this file?


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1144 
PS1, Line 1144: 
1137 | struct func_type
     | ...
1139 |     /* * The calling convention for targets supporting multiple ABIs.
1140 |        Right now this is only fetched from the Dwarf-2
1141 |        DW_AT_calling_convention attribute.  The value is one of the
1142 |        DW_CC enum dwarf_calling_convention constants.  */
1143 | 
1144 >     unsigned calling_convention : 8;
1145 | 
1146 |     /* * Whether this function normally returns to its caller.  It is
1147 |        set from the DW_AT_noreturn attribute if set on the
1148 |        DW_TAG_subprogram.  */
1149 | 

This is an existing definition I had aimed to be consistent with.
Simon Marchi (Code Review) Oct. 31, 2019, 2:10 p.m. UTC | #6 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101: 
 986 | struct cplus_struct_type
     | ...
1096 | 
1097 |     /* * The calling convention for this type, fetched from the
1098 |        DW_AT_calling_convention attribute.  The value is one of the
1099 |        DW_CC enum dwarf_calling_convention constants.  */
1100 | 
1101 >     unsigned calling_convention : 8;
1102 |   };
1103 | 
1104 | /* * Struct used to store conversion rankings.  */
1105 | 
1106 | struct rank

> I assume this comment applies to the "defaulted" attribute, too. […]

Ah, thanks for pointing out this other usage in func_type.  Well, if I was reviewing that, I would give the same comment there too.

If we're going to document that the field takes one of the values of the dwarf_calling_convention enum, we're already depending on it implicitly.  Making it explicit just makes it clearer, IMO.

If you agree, I think we can change this other spot in another patch to keep it consistent.
Simon Marchi (Code Review) Oct. 31, 2019, 5:22 p.m. UTC | #7 
Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 1:

(5 comments)

> I assume this is used elsewhere in your series.  Now that we're on gerrit and can't
> do cover letters, I think it would be helpful if supporting patches like this could
> mention facts like that... something for us all to consider as we write commit
> messages.

Yes.  I update the commit message in v2.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/dwarf2read.c@15533 
PS1, Line 15533: 
15422 | dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
      | ...
15528 |   if (attr && DW_UNSND (attr) != 0)
15529 |     fnp->is_artificial = 1;
15530 | 
15531 |   /* Check for defaulted methods.  */
15532 |   attr = dwarf2_attr (die, DW_AT_defaulted, cu);
15533 >   if (attr)
15534 |     fnp->defaulted = DW_UNSND (attr);
15535 | 
15536 |   /* Check for deleted methods.  */
15537 |   attr = dwarf2_attr (die, DW_AT_deleted, cu);
15538 |   if (attr && DW_UNSND (attr) != 0)

> Use: […]

Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/dwarf2read.c@15534 
PS1, Line 15534: 
15422 | dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
      | ...
15529 |     fnp->is_artificial = 1;
15530 | 
15531 |   /* Check for defaulted methods.  */
15532 |   attr = dwarf2_attr (die, DW_AT_defaulted, cu);
15533 |   if (attr)
15534 >     fnp->defaulted = DW_UNSND (attr);
15535 | 
15536 |   /* Check for deleted methods.  */
15537 |   attr = dwarf2_attr (die, DW_AT_deleted, cu);
15538 |   if (attr && DW_UNSND (attr) != 0)
15539 |     fnp->is_deleted = 1;

> Shouldn't we do a bit of validation here, making sure `attr` is one of the valid values, issuing a c […]

Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@929 
PS1, Line 929: 
899 | struct fn_field
    | ...
924 |   struct type *fcontext;
925 | 
926 |   /* * DW_AT_defaulted attribute for this function; the value is one
927 |      of the DW_DEFAULTED enum dwarf_defaulted_attribute constants.  */
928 | 
929 >   unsigned int defaulted;
930 | 
931 |   /* Attributes.  */
932 | 
933 |   unsigned int is_const:1;
934 |   unsigned int is_volatile:1;

> Should this be a bit-field or a narrower integer type? […]

In v2, I make this 2-bit-wide and move down for better packing.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@929 
PS1, Line 929: 
899 | struct fn_field
    | ...
924 |   struct type *fcontext;
925 | 
926 |   /* * DW_AT_defaulted attribute for this function; the value is one
927 |      of the DW_DEFAULTED enum dwarf_defaulted_attribute constants.  */
928 | 
929 >   unsigned int defaulted;
930 | 
931 |   /* Attributes.  */
932 | 
933 |   unsigned int is_const:1;
934 |   unsigned int is_volatile:1;

> It would be good for this field to be an enum. […]

In v2, I make this an ENUM_BITFIELD.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1101 
PS1, Line 1101: 
 986 | struct cplus_struct_type
     | ...
1096 | 
1097 |     /* * The calling convention for this type, fetched from the
1098 |        DW_AT_calling_convention attribute.  The value is one of the
1099 |        DW_CC enum dwarf_calling_convention constants.  */
1100 | 
1101 >     unsigned calling_convention : 8;
1102 |   };
1103 | 
1104 | /* * Struct used to store conversion rankings.  */
1105 | 
1106 | struct rank

> Ah, thanks for pointing out this other usage in func_type. […]

Yes, agree. In v2, I make this an ENUM_BITFIELD and move near `is_dynamic`.
Simon Marchi (Code Review) Oct. 31, 2019, 5:57 p.m. UTC | #8 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 2:

(3 comments)

Thanks for the update.  Just two minor comments about the validation.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15831 
PS2, Line 15831: 
15822 | is_valid_DW_AT_calling_convention (ULONGEST value)
      | ...
15826 |     case DW_CC_normal:
15827 |     case DW_CC_program:
15828 |     case DW_CC_nocall:
15829 |     case DW_CC_pass_by_reference:
15830 |     case DW_CC_pass_by_value:
15831 >     case DW_CC_lo_user:
15832 >     case DW_CC_hi_user:
15833 |     /* case DW_CC_GNU_renesas_sh: Duplicate of DW_CC_lo_user.  */
15834 |     case DW_CC_GNU_borland_fastcall_i386:
15835 |     /* case DW_CC_GDB_IBM_OpenCL: Duplicate of DW_CC_hi_user.  */
15836 |       return true;
15837 |     }

I don' think we should include lo_user and hi_user in the switch, since they are not "real" values.  They just indicate the range for vendor-specific values.

DW_CC_GNU_renesas_sh and DW_CC_GDB_IBM_OpenCL are "real" values, so they should be included in the switch.  In the end, the behavior will be the same, since as you noted, they are duplicates of lo_user and hi_user.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15928 
PS2, Line 15928: 
15857 | read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      | ...
15923 |     TYPE_DECLARED_CLASS (type) = 1;
15924 | 
15925 |   /* Store the calling convention in the type if it's available in
15926 |      the die.  Otherwise the calling convention remains set to
15927 |      the default value DW_CC_normal.  */
15928 >   attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
15929 |   if (attr != nullptr
15930 |       && is_valid_DW_AT_calling_convention (DW_UNSND (attr)))
15931 |     {
15932 |       ALLOCATE_CPLUS_STRUCT_TYPE (type);
15933 |       TYPE_CPLUS_CALLING_CONVENTION (type)

According to table 5.5 of DWARF5, the acceptable calling convention values for types are restricted to:

- DW_CC_normal
- DW_CC_pass_by_value
- DW_CC_pass_by_reference

So should we complain and reject the value if it's not one of those? The idea is that if a producer emits an unacceptable value by mistake, the issue is caught as early as possible rather than silently accepted.

In the spot where we read DW_AT_calling_convention for subroutines, we could validate that the value is one of those listed in table 3.3:

- DW_CC_normal
- DW_CC_program
- DW_CC_nocall

... plus the vendor-specific values that we support.  So it might be simpler to do two functions, is_valid_DW_AT_calling_convention_for_types and is_valid_DW_AT_calling_convention_for_subprogram. Therefore, in this patch I suggest calling the new function "is_valid_DW_AT_calling_convention_for_types" to leave room to do the same thing for subprograms.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h 
File gdb/gdbtypes.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/1/gdb/gdbtypes.h@1144 
PS1, Line 1144: 
1137 | struct func_type
     | ...
1139 |     /* * The calling convention for targets supporting multiple ABIs.
1140 |        Right now this is only fetched from the Dwarf-2
1141 |        DW_AT_calling_convention attribute.  The value is one of the
1142 |        DW_CC enum dwarf_calling_convention constants.  */
1143 | 
1144 >     unsigned calling_convention : 8;
1145 | 
1146 |     /* * Whether this function normally returns to its caller.  It is
1147 |        set from the DW_AT_noreturn attribute if set on the
1148 |        DW_TAG_subprogram.  */
1149 | 

> This is an existing definition I had aimed to be consistent with.

Ack, let's change it in another patch.
Simon Marchi (Code Review) Oct. 31, 2019, 8:12 p.m. UTC | #9 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 3:

(2 comments)

Thanks, this patch LGTM for my part.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15831 
PS2, Line 15831: 
15822 | is_valid_DW_AT_calling_convention (ULONGEST value)
      | ...
15826 |     case DW_CC_normal:
15827 |     case DW_CC_program:
15828 |     case DW_CC_nocall:
15829 |     case DW_CC_pass_by_reference:
15830 |     case DW_CC_pass_by_value:
15831 >     case DW_CC_lo_user:
15832 >     case DW_CC_hi_user:
15833 |     /* case DW_CC_GNU_renesas_sh: Duplicate of DW_CC_lo_user.  */
15834 |     case DW_CC_GNU_borland_fastcall_i386:
15835 |     /* case DW_CC_GDB_IBM_OpenCL: Duplicate of DW_CC_hi_user.  */
15836 |       return true;
15837 |     }

> I don' think we should include lo_user and hi_user in the switch, since they are not "real" values. […]

Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15928 
PS2, Line 15928: 
15857 | read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      | ...
15923 |     TYPE_DECLARED_CLASS (type) = 1;
15924 | 
15925 |   /* Store the calling convention in the type if it's available in
15926 |      the die.  Otherwise the calling convention remains set to
15927 |      the default value DW_CC_normal.  */
15928 >   attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
15929 |   if (attr != nullptr
15930 |       && is_valid_DW_AT_calling_convention (DW_UNSND (attr)))
15931 |     {
15932 |       ALLOCATE_CPLUS_STRUCT_TYPE (type);
15933 |       TYPE_CPLUS_CALLING_CONVENTION (type)

> According to table 5. […]

Done
Simon Marchi (Code Review) Oct. 31, 2019, 8:12 p.m. UTC | #10 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 3: Code-Review+2
Simon Marchi (Code Review) Oct. 31, 2019, 8:12 p.m. UTC | #11 
Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 3:

(3 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15831 
PS2, Line 15831: 
15822 | is_valid_DW_AT_calling_convention (ULONGEST value)
      | ...
15826 |     case DW_CC_normal:
15827 |     case DW_CC_program:
15828 |     case DW_CC_nocall:
15829 |     case DW_CC_pass_by_reference:
15830 |     case DW_CC_pass_by_value:
15831 >     case DW_CC_lo_user:
15832 >     case DW_CC_hi_user:
15833 |     /* case DW_CC_GNU_renesas_sh: Duplicate of DW_CC_lo_user.  */
15834 |     case DW_CC_GNU_borland_fastcall_i386:
15835 |     /* case DW_CC_GDB_IBM_OpenCL: Duplicate of DW_CC_hi_user.  */
15836 |       return true;
15837 |     }

> I don' think we should include lo_user and hi_user in the switch, since they are not "real" values. […]

Ack


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/2/gdb/dwarf2read.c@15928 
PS2, Line 15928: 
15857 | read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      | ...
15923 |     TYPE_DECLARED_CLASS (type) = 1;
15924 | 
15925 |   /* Store the calling convention in the type if it's available in
15926 |      the die.  Otherwise the calling convention remains set to
15927 |      the default value DW_CC_normal.  */
15928 >   attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
15929 |   if (attr != nullptr
15930 |       && is_valid_DW_AT_calling_convention (DW_UNSND (attr)))
15931 |     {
15932 |       ALLOCATE_CPLUS_STRUCT_TYPE (type);
15933 |       TYPE_CPLUS_CALLING_CONVENTION (type)

> According to table 5. […]

Yes, makes much sense.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/3/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/3/gdb/dwarf2read.c@15452 
PS3, Line 15452: 
15444 | is_valid_DW_AT_defaulted (ULONGEST value)
      | ...
15447 |     {
15448 |     case DW_DEFAULTED_no:
15449 |     case DW_DEFAULTED_in_class:
15450 |     case DW_DEFAULTED_out_of_class:
15451 |       return true;
15452 >     }
15453 | 
15454 |   complaint (_("unrecognized DW_AT_defaulted value (%lu)"), value);
15455 |   return false;
15456 | }
15457 | 

I did not put a "default" case here whereas in 'is_valid_DW_AT_calling_convention_for_type' there is one.  This is intentional.  In case a new enum constant is added to 'dwarf_defaulted_attribute', a friendly compiler may warn about an uncaught case here.  This was not possible in 'is_valid_DW_AT_calling_convention_for_type' as some values are already not valid.
Simon Marchi (Code Review) Oct. 31, 2019, 8:18 p.m. UTC | #12 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135
......................................................................


Patch Set 3:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/3/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/135/3/gdb/dwarf2read.c@15452 
PS3, Line 15452: 
15444 | is_valid_DW_AT_defaulted (ULONGEST value)
      | ...
15447 |     {
15448 |     case DW_DEFAULTED_no:
15449 |     case DW_DEFAULTED_in_class:
15450 |     case DW_DEFAULTED_out_of_class:
15451 |       return true;
15452 >     }
15453 | 
15454 |   complaint (_("unrecognized DW_AT_defaulted value (%lu)"), value);
15455 |   return false;
15456 | }
15457 | 

> I did not put a "default" case here whereas in 'is_valid_DW_AT_calling_convention_for_type' there is […]

I think this is fine, thanks.
diff mbox

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3a88c45..463d9f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -15528,6 +15528,16 @@ 
   if (attr && DW_UNSND (attr) != 0)
     fnp->is_artificial = 1;
 
+  /* Check for defaulted methods.  */
+  attr = dwarf2_attr (die, DW_AT_defaulted, cu);
+  if (attr)
+    fnp->defaulted = DW_UNSND (attr);
+
+  /* Check for deleted methods.  */
+  attr = dwarf2_attr (die, DW_AT_deleted, cu);
+  if (attr && DW_UNSND (attr) != 0)
+    fnp->is_deleted = 1;
+
   fnp->is_constructor = dwarf2_is_constructor (die, cu);
 
   /* Get index in virtual function table if it is a virtual member
@@ -15848,6 +15858,16 @@ 
   if (cu->language == language_cplus && die->tag == DW_TAG_class_type)
     TYPE_DECLARED_CLASS (type) = 1;
 
+  /* Store the calling convention in the type if it's available in
+     the die.  Otherwise the calling convention remains set to
+     the default value DW_CC_normal.  */
+  attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
+  if (attr)
+    {
+      ALLOCATE_CPLUS_STRUCT_TYPE (type);
+      TYPE_CPLUS_CALLING_CONVENTION (type) = DW_UNSND (attr);
+    }
+
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr)
     {
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index fd1c765..d5448cf 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4394,6 +4394,10 @@ 
 			    TYPE_FN_FIELD_PROTECTED (f, overload_idx));
 	  printfi_filtered (spaces + 8, "is_stub %d\n",
 			    TYPE_FN_FIELD_STUB (f, overload_idx));
+	  printfi_filtered (spaces + 8, "defaulted %d\n",
+			    TYPE_FN_FIELD_DEFAULTED (f, overload_idx));
+	  printfi_filtered (spaces + 8, "is_deleted %d\n",
+			    TYPE_FN_FIELD_DELETED (f, overload_idx));
 	  printfi_filtered (spaces + 8, "voffset %u\n",
 			    TYPE_FN_FIELD_VOFFSET (f, overload_idx));
 	}
@@ -4457,6 +4461,9 @@ 
     {
       dump_fn_fieldlists (type, spaces);
     }
+
+  printfi_filtered (spaces, "calling_convention %d\n",
+		    TYPE_CPLUS_CALLING_CONVENTION (type));
 }
 
 /* Print the contents of the TYPE's type_specific union, assuming that
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index d431cb6..643122a 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -923,6 +923,11 @@ 
 
   struct type *fcontext;
 
+  /* * DW_AT_defaulted attribute for this function; the value is one
+     of the DW_DEFAULTED enum dwarf_defaulted_attribute constants.  */
+
+  unsigned int defaulted;
+
   /* Attributes.  */
 
   unsigned int is_const:1;
@@ -940,9 +945,13 @@ 
 
   unsigned int is_constructor : 1;
 
+  /* * True if this function is deleted, false otherwise.  */
+
+  unsigned int is_deleted : 1;
+
   /* * Unused.  */
 
-  unsigned int dummy:9;
+  unsigned int dummy:8;
 
   /* * Index into that baseclass's virtual function table, minus 2;
      else if static: VOFFSET_STATIC; else: 0.  */
@@ -1084,6 +1093,12 @@ 
        classes.  */
 
     struct symbol **template_arguments;
+
+    /* * The calling convention for this type, fetched from the
+       DW_AT_calling_convention attribute.  The value is one of the
+       DW_CC enum dwarf_calling_convention constants.  */
+
+    unsigned calling_convention : 8;
   };
 
 /* * Struct used to store conversion rankings.  */
@@ -1414,6 +1429,8 @@ 
     ? (struct cplus_struct_type*)&cplus_struct_default \
     : TYPE_RAW_CPLUS_SPECIFIC(thistype))
 #define TYPE_RAW_CPLUS_SPECIFIC(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.cplus_stuff
+#define TYPE_CPLUS_CALLING_CONVENTION(thistype) \
+  TYPE_MAIN_TYPE(thistype)->type_specific.cplus_stuff->calling_convention
 #define TYPE_FLOATFORMAT(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.floatformat
 #define TYPE_GNAT_SPECIFIC(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.gnat_stuff
 #define TYPE_DESCRIPTIVE_TYPE(thistype) TYPE_GNAT_SPECIFIC(thistype)->descriptive_type
@@ -1530,6 +1547,8 @@ 
 #define TYPE_FN_FIELD_VOFFSET(thisfn, n) ((thisfn)[n].voffset-2)
 #define TYPE_FN_FIELD_VIRTUAL_P(thisfn, n) ((thisfn)[n].voffset > 1)
 #define TYPE_FN_FIELD_STATIC_P(thisfn, n) ((thisfn)[n].voffset == VOFFSET_STATIC)
+#define TYPE_FN_FIELD_DEFAULTED(thisfn, n) ((thisfn)[n].defaulted)
+#define TYPE_FN_FIELD_DELETED(thisfn, n) ((thisfn)[n].is_deleted)
 
 /* Accessors for typedefs defined by a class.  */
 #define TYPE_TYPEDEF_FIELD_ARRAY(thistype) \