Record and output access specifiers for nested typedefs

  We currently do not record access information for typedefs defined inside
classes.  Consider:

struct foo
{
   typedef int PUBLIC;
 private:
   typedef int PRIVATE;
   PRIVATE b;
};

(gdb) ptype foo
type = struct foo {
  private:
    PRIVATE b;

    typedef int PRIVATE;
    typedef int PUBLIC;
}

This patch fixes this:

(gdb) ptype foo
type = struct foo {
  private:
    PRIVATE b;

    typedef int PRIVATE;
  public:
    typedef int PUBLIC;
}

gdb/ChangeLog:

	* c-typeprint.c (enum access_specifier): Moved here from
	c_type_print_base.
	(output_access_specifier): New function.
	(c_type_print_base): Consider typedefs when assessing
	whether access labels are needed.
	Use output_access_specifier as needed.
	Output access specifier for typedefs, if needed.
	* dwarf2read.c (dwarf2_add_typedef): Record DW_AT_accessibility.
	* gdbtypes.h (struct typedef_field) <is_protected, is_private>: New
	fields.
	(TYPE_TYPEDEF_FIELD_PROTECTED, TYPE_TYEPDEF_FIELD_PRIVATE): New
	accessor macros.

gdb/testsuite/ChangeLog:

	* gdb.cp/classes.cc (class_with_typedefs, class_with_public_typedef)
	(class_with_protected_typedef, class_with_private_typedef)
	(struct_with_public_typedef, struct_with_protected_typedef)
	(struct_with_private_typedef): New classes/structs.
	* gdb.cp/classes.exp (test_ptype_class_objects): Add tests for
	typedefs and access specifiers.
---
 gdb/c-typeprint.c                | 145 +++++++++++++++++++++++----------------
 gdb/dwarf2read.c                 |  22 ++++++
 gdb/gdbtypes.h                   |  11 +++
 gdb/testsuite/gdb.cp/classes.cc  |  83 ++++++++++++++++++++++
 gdb/testsuite/gdb.cp/classes.exp |  58 ++++++++++++++++
 5 files changed, 259 insertions(+), 60 deletions(-)
  

On 2017-10-13 03:18 PM, Keith Seitz wrote:
> We currently do not record access information for typedefs defined inside
> classes.  Consider:
> 
> struct foo
> {
>    typedef int PUBLIC;
>  private:
>    typedef int PRIVATE;
>    PRIVATE b;
> };
> 
> (gdb) ptype foo
> type = struct foo {
>   private:
>     PRIVATE b;
> 
>     typedef int PRIVATE;
>     typedef int PUBLIC;
> }
> 
> This patch fixes this:
> 
> (gdb) ptype foo
> type = struct foo {
>   private:
>     PRIVATE b;
> 
>     typedef int PRIVATE;
>   public:
>     typedef int PUBLIC;
> }

Hi Keith,

I just have a few minor comments.

> gdb/ChangeLog:
> 
> 	* c-typeprint.c (enum access_specifier): Moved here from
> 	c_type_print_base.
> 	(output_access_specifier): New function.
> 	(c_type_print_base): Consider typedefs when assessing
> 	whether access labels are needed.
> 	Use output_access_specifier as needed.
> 	Output access specifier for typedefs, if needed.
> 	* dwarf2read.c (dwarf2_add_typedef): Record DW_AT_accessibility.
> 	* gdbtypes.h (struct typedef_field) <is_protected, is_private>: New
> 	fields.
> 	(TYPE_TYPEDEF_FIELD_PROTECTED, TYPE_TYEPDEF_FIELD_PRIVATE): New

"TYPE_TYEPDEF_FIELD_PRIVATE"

> 	accessor macros.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.cp/classes.cc (class_with_typedefs, class_with_public_typedef)
> 	(class_with_protected_typedef, class_with_private_typedef)
> 	(struct_with_public_typedef, struct_with_protected_typedef)
> 	(struct_with_private_typedef): New classes/structs.
> 	* gdb.cp/classes.exp (test_ptype_class_objects): Add tests for
> 	typedefs and access specifiers.
> ---
>  gdb/c-typeprint.c                | 145 +++++++++++++++++++++++----------------
>  gdb/dwarf2read.c                 |  22 ++++++
>  gdb/gdbtypes.h                   |  11 +++
>  gdb/testsuite/gdb.cp/classes.cc  |  83 ++++++++++++++++++++++
>  gdb/testsuite/gdb.cp/classes.exp |  58 ++++++++++++++++
>  5 files changed, 259 insertions(+), 60 deletions(-)
> 
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 978fcc4..3dc74f9 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -32,6 +32,16 @@
>  #include "cp-abi.h"
>  #include "cp-support.h"
>  
> +/* A list of access specifiers used for printing.  */
> +
> +enum access_specifier
> +{
> +  s_none,
> +  s_public,
> +  s_private,
> +  s_protected
> +};
> +
>  static void c_type_print_varspec_prefix (struct type *,
>  					 struct ui_file *,
>  					 int, int, int,
> @@ -826,7 +836,42 @@ c_type_print_template_args (const struct type_print_options *flags,
>      fputs_filtered (_("] "), stream);
>  }
>  
> -/* Print the name of the type (or the ultimate pointer target,
> +/* Output an access specifier to STREAM, if needed.  */
> +
> +static void
> +output_access_specifier (struct ui_file *stream, enum access_specifier &access,
> +			 int level, bool is_protected, bool is_private)

I would suggest renaming access it to last_access or previous_access.  Could you also document
it in the function comment?

It might also be clearer to have output_access_specifier return the access_specifier,
rather than modifying the passed value directly.  Otherwise, it's not really obvious
what's happening in the caller.

> +{
> +  if (is_protected)
> +    {
> +      if (access != s_protected)
> +	{
> +	  access = s_protected;
> +	  fprintfi_filtered (level + 2, stream,
> +			     "protected:\n");
> +	}
> +    }
> +  else if (is_private)
> +    {
> +      if (access != s_private)
> +	{
> +	  access = s_private;
> +	  fprintfi_filtered (level + 2, stream,
> +			     "private:\n");
> +	}
> +    }
> +  else
> +    {
> +      if (access != s_public)
> +	{
> +	  access = s_public;
> +	  fprintfi_filtered (level + 2, stream,
> +			     "public:\n");
> +	}
> +    }
> +}
> +
> +  /* Print the name of the type (or the ultimate pointer target,

Spurious indent change on that last line.

>     function value or array element), or the description of a structure
>     or union.
>  
> @@ -850,11 +895,7 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  {
>    int i;
>    int len, real_len;
> -  enum
> -    {
> -      s_none, s_public, s_private, s_protected
> -    }
> -  section_type;
> +  enum access_specifier section_type;
>    int need_access_label = 0;
>    int j, len2;
>  
> @@ -1034,6 +1075,18 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  			  break;
>  		      }
>  		  }
> +		QUIT;
> +		if (!need_access_label)
> +		  {
> +		    for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> +		      {
> +			if (!TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> +			  {
> +			    need_access_label = 1;
> +			    break;
> +			  }
> +		      }
> +		  }
>  	      }
>  	    else
>  	      {
> @@ -1068,6 +1121,19 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  			  break;
>  		      }
>  		  }
> +		QUIT;
> +		if (!need_access_label)
> +		  {
> +		    for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> +		      {
> +			if (TYPE_TYPEDEF_FIELD_PROTECTED (type, i)
> +			    || TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> +			  {
> +			    need_access_label = 1;
> +			    break;
> +			  }
> +		      }
> +		  }
>  	      }
>  
>  	    /* If there is a base class for this type,
> @@ -1088,33 +1154,9 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  
>  		if (need_access_label)
>  		  {
> -		    if (TYPE_FIELD_PROTECTED (type, i))
> -		      {
> -			if (section_type != s_protected)
> -			  {
> -			    section_type = s_protected;
> -			    fprintfi_filtered (level + 2, stream,
> -					       "protected:\n");
> -			  }
> -		      }
> -		    else if (TYPE_FIELD_PRIVATE (type, i))
> -		      {
> -			if (section_type != s_private)
> -			  {
> -			    section_type = s_private;
> -			    fprintfi_filtered (level + 2, stream,
> -					       "private:\n");
> -			  }
> -		      }
> -		    else
> -		      {
> -			if (section_type != s_public)
> -			  {
> -			    section_type = s_public;
> -			    fprintfi_filtered (level + 2, stream,
> -					       "public:\n");
> -			  }
> -		      }
> +		    output_access_specifier (stream, section_type, level,
> +					     TYPE_FIELD_PROTECTED (type, i),
> +					     TYPE_FIELD_PRIVATE (type, i));
>  		  }
>  
>  		print_spaces_filtered (level + 4, stream);
> @@ -1188,33 +1230,9 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  		  inner_cleanup = make_cleanup (null_cleanup, NULL);
>  
>  		  QUIT;
> -		  if (TYPE_FN_FIELD_PROTECTED (f, j))
> -		    {
> -		      if (section_type != s_protected)
> -			{
> -			  section_type = s_protected;
> -			  fprintfi_filtered (level + 2, stream,
> -					     "protected:\n");
> -			}
> -		    }
> -		  else if (TYPE_FN_FIELD_PRIVATE (f, j))
> -		    {
> -		      if (section_type != s_private)
> -			{
> -			  section_type = s_private;
> -			  fprintfi_filtered (level + 2, stream,
> -					     "private:\n");
> -			}
> -		    }
> -		  else
> -		    {
> -		      if (section_type != s_public)
> -			{
> -			  section_type = s_public;
> -			  fprintfi_filtered (level + 2, stream,
> -					     "public:\n");
> -			}
> -		    }
> +		  output_access_specifier (stream, section_type, level,
> +					   TYPE_FN_FIELD_PROTECTED (f, j),
> +					   TYPE_FN_FIELD_PRIVATE (f, j));
>  
>  		  print_spaces_filtered (level + 4, stream);
>  		  if (TYPE_FN_FIELD_VIRTUAL_P (f, j))
> @@ -1326,6 +1344,13 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  		  gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
>  		  target = TYPE_TARGET_TYPE (target);
>  
> +		  if (need_access_label)
> +		    {
> +		      output_access_specifier
> +			(stream, section_type, level,
> +			 TYPE_TYPEDEF_FIELD_PROTECTED (type, i),
> +			 TYPE_TYPEDEF_FIELD_PRIVATE (type, i));
> +		    }
>  		  print_spaces_filtered (level + 4, stream);
>  		  fprintf_filtered (stream, "typedef ");
>  
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 9bdb75d..c0200f9 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -13274,6 +13274,28 @@ dwarf2_add_typedef (struct field_info *fip, struct die_info *die,
>  
>    fp->type = read_type_die (die, cu);
>  
> +  /* Save accessibility.  */
> +  enum dwarf_access_attribute accessibility;
> +  struct attribute *attr = dwarf2_attr (die, DW_AT_accessibility, cu);
> +  if (attr != NULL)
> +    accessibility = (enum dwarf_access_attribute) DW_UNSND (attr);
> +  else
> +    accessibility = dwarf2_default_access_attribute (die, cu);
> +  switch (accessibility)
> +    {
> +    case DW_ACCESS_public:
> +      /* The assumed value if neither private nor protected.  */
> +      break;
> +    case DW_ACCESS_private:
> +      fp->is_private = 1;
> +      break;
> +    case DW_ACCESS_protected:
> +      fp->is_protected = 1;
> +      break;
> +    default:
> +      gdb_assert_not_reached ("unexpected accessibility attribute");
> +    }
> +
>    new_field->next = fip->typedef_field_list;
>    fip->typedef_field_list = new_field;
>    fip->typedef_field_list_count++;
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 0e0aead..48af6f1 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -896,6 +896,12 @@ struct typedef_field
>    /* * Type this typedef named NAME represents.  */
>  
>    struct type *type;
> +
> +  /* * True if this field was declared protected, false otherwise.  */
> +  unsigned int is_protected : 1;
> +
> +  /* * True if this field was declared private, false otherwise.  */
> +  unsigned int is_private : 1;
>  };
>  
>  /* * C++ language-specific information for TYPE_CODE_STRUCT and
> @@ -1427,6 +1433,7 @@ extern void set_type_vptr_basetype (struct type *, struct type *);
>  #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)
>  
> +/* Accessors for typedefs defined by a class.  */
>  #define TYPE_TYPEDEF_FIELD_ARRAY(thistype) \
>    TYPE_CPLUS_SPECIFIC (thistype)->typedef_field
>  #define TYPE_TYPEDEF_FIELD(thistype, n) \
> @@ -1437,6 +1444,10 @@ extern void set_type_vptr_basetype (struct type *, struct type *);
>    TYPE_TYPEDEF_FIELD (thistype, n).type
>  #define TYPE_TYPEDEF_FIELD_COUNT(thistype) \
>    TYPE_CPLUS_SPECIFIC (thistype)->typedef_field_count
> +#define TYPE_TYPEDEF_FIELD_PROTECTED(thistype, n) \
> +  TYPE_TYPEDEF_FIELD (thistype, n).is_protected
> +#define TYPE_TYPEDEF_FIELD_PRIVATE(thistype, n)        \
> +  TYPE_TYPEDEF_FIELD (thistype, n).is_private
>  
>  #define TYPE_IS_OPAQUE(thistype) \
>    (((TYPE_CODE (thistype) == TYPE_CODE_STRUCT) \
> diff --git a/gdb/testsuite/gdb.cp/classes.cc b/gdb/testsuite/gdb.cp/classes.cc
> index 2a81473..50f0740 100644
> --- a/gdb/testsuite/gdb.cp/classes.cc
> +++ b/gdb/testsuite/gdb.cp/classes.cc
> @@ -545,6 +545,82 @@ small::method ()
>    return x + 5;
>  }
>  
> +class class_with_typedefs
> +{
> +public:
> +  typedef int public_int;
> +protected:
> +  typedef int protected_int;
> +private:
> +  typedef int private_int;
> +
> +public:
> +  class_with_typedefs ()
> +    : public_int_ (1), protected_int_ (2), private_int_ (3) {}
> +  public_int add_public (public_int a) { return a + public_int_; }
> +  public_int add_all (int a)
> +  { return add_public (a) + add_protected (a) + add_private (a); }
> +
> +protected:
> +  protected_int add_protected (protected_int a) { return a + protected_int_; }
> +
> +private:
> +  private_int add_private (private_int a) { return a + private_int_; }
> +
> +protected:
> +  public_int public_int_;
> +  protected_int protected_int_;
> +  private_int private_int_;
> +};
> +
> +class class_with_public_typedef
> +{
> +  int a;
> +public:
> +  typedef int INT;
> +  INT b;
> +};
> +
> +class class_with_protected_typedef
> +{
> +  int a;
> +protected:
> +  typedef int INT;
> +  INT b;
> +};
> +
> +class class_with_private_typedef
> +{
> +  int a;
> +private:
> +  typedef int INT;
> +  INT b;
> +};
> +
> +struct struct_with_public_typedef
> +{
> +  int a;
> +public:
> +  typedef int INT;
> +  INT b;
> +};
> +
> +struct struct_with_protected_typedef
> +{
> +  int a;
> +protected:
> +  typedef int INT;
> +  INT b;
> +};
> +
> +struct struct_with_private_typedef
> +{
> +  int a;
> +private:
> +  typedef int INT;
> +  INT b;
> +};
> +
>  void marker_reg1 () {}
>  
>  int
> @@ -624,3 +700,10 @@ protected_class protected_c;
>  default_private_class default_private_c;
>  explicit_private_class explicit_private_c;
>  mixed_protection_class mixed_protection_c;
> +class_with_typedefs class_with_typedefs_c;
> +class_with_public_typedef class_with_public_typedef_c;
> +class_with_protected_typedef class_with_protected_typedef_c;
> +class_with_private_typedef class_with_private_typedef_c;
> +struct_with_public_typedef struct_with_public_typedef_s;
> +struct_with_protected_typedef struct_with_protected_typedef_s;
> +struct_with_private_typedef struct_with_private_typedef_s;
> diff --git a/gdb/testsuite/gdb.cp/classes.exp b/gdb/testsuite/gdb.cp/classes.exp
> index 256fa68..1f6d377 100644
> --- a/gdb/testsuite/gdb.cp/classes.exp
> +++ b/gdb/testsuite/gdb.cp/classes.exp
> @@ -316,6 +316,64 @@ proc test_ptype_class_objects {} {
>  	    { field  public "int y;" }
>  	    { method public "DynamicBar(int, int);" }
>  	}
> +
> +    # Classes with tyepdefs of different access.

"tyepdefs".

> +
> +    cp_test_ptype_class \
> +	"class class_with_typedefs" "" "class" "class_with_typedefs" \
> +	{
> +	    { field protected \
> +		  "class_with_typedefs::public_int public_int_;" }
> +	    { field protected \
> +		  "class_with_typedefs::protected_int protected_int_;" }
> +	    { field protected \
> +		  "class_with_typedefs::private_int private_int_;" }
> +	    { method public "class_with_typedefs(void);" }
> +	    { method public "class_with_typedefs::public_int add_public(class_with_typedefs::public_int);" }
> +	    { method public \
> +		  "class_with_typedefs::public_int add_all(int);" }
> +	    { method protected "class_with_typedefs::protected_int add_protected(class_with_typedefs::protected_int);" }
> +	    { method private "class_with_typedefs::private_int add_private(class_with_typedefs::private_int);" }
> +	    {typedef public "typedef int public_int;" }
> +	    {typedef protected "typedef int protected_int;" }
> +	    {typedef private "typedef int private_int;" }

Add spaces after {, for consistency.

> +	}
> +
> +    cp_test_ptype_class \
> +	"class class_with_public_typedef" "" "class" \
> +	"class_with_public_typedef" {
> +	    { field private "int a;" }
> +	    { field public "class_with_public_typedef::INT b;" }
> +	    { typedef public "typedef int INT;" }
> +	}
> +    cp_test_ptype_class \
> +	"class class_with_protected_typedef" "" "class" \
> +	"class_with_protected_typedef" {
> +	    { field private "int a;" }
> +	    { field protected "class_with_protected_typedef::INT b;" }
> +	    { typedef protected "typedef int INT;" }
> +	}
> +    cp_test_ptype_class \
> +	"struct struct_with_protected_typedef" "" "struct" \
> +	"struct_with_protected_typedef" {
> +	    { field public "int a;" }
> +	    { field protected "struct_with_protected_typedef::INT b;" }
> +	    { typedef protected "typedef int INT;" }
> +	}
> +    cp_test_ptype_class \
> +	"struct struct_with_private_typedef" "" "struct" \
> +	"struct_with_private_typedef" {
> +	    { field public "int a;" }
> +	    { field private "struct_with_private_typedef::INT b;" }
> +	    { typedef private "typedef int INT;" }
> +	}
> +
> +    # For the following two cases, we cannot use cp_test_ptype_class.
> +    # We need to explicitly check whether the access label was suppressed.
> +    set ws {[\ \t\r\n]*}

I don't think it's necessary to escape the space.

> +    foreach {tag lbl} {"class" "private" "struct" "public"} {
> +	gdb_test "ptype/r ${tag}_with_${lbl}_typedef" "type = $tag ${tag}_with_${lbl}_typedef \{${ws}int a;${ws}${tag}_with_${lbl}_typedef::INT b;${ws}typedef int INT;${ws}\}"
> +    }
>  }
>  
>  # Test simple access to class members.
> 

Thanks!

Simon