Re: [PATCH] Record/output access specifiers for class typedefs

[Pedro Alves <palves@redhat.com>]

On 06/25/2017 07:52 PM, Keith Seitz wrote:
> We do not currently record/output accessibility for typedefs defined
> in a class:
> 
> (gdb) list
> 71	struct typedef_struct {
> 72	public:
> 73	  typedef int public_int;
> 74	  public_int a;
> 75	protected:
> 76	  typedef int protected_int;
> 77	  protected_int b;
> 78	private:
> 79	  typedef int private_int;
> 80	  private_int c;
> 81	protected:
> 82	  typedef float protected_float;
> 83	  protected_float d;
> 84	private:
> 85	  typedef float private_float;
> 86	  private_float e;
> 87	public:
> 88	  typedef float public_float;
> 89	  public_float f;
> 90	};
> (gdb) ptype typedef_struct
> type = struct typedef_struct {
>   public:
>     public_int a;
>   protected:
>     public_int b;
>   private:
>     public_int c;
>   protected:
>     protected_float d;
>   private:
>     protected_float e;
>   public:
>     protected_float f;
> 
>     typedef int public_int;
>     typedef int protected_int;
>     typedef int private_int;
>     typedef float protected_float;
>     typedef float private_float;
>     typedef float public_float;
> }
> 
> This patch modifies the DWARF reader to record accessibility when reading
> in typedef DIEs.

As general principle, please also show in the commit log what
output looks like after the patch.


On 06/25/2017 07:52 PM, Keith Seitz wrote:

> +  /* Save accessibility.  */
> +  struct attribute *attr = dwarf2_attr (die, DW_AT_accessibility, cu);
> +  enum dwarf_access_attribute accessibility;
> +
> +  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:
> +      fp->is_public = 1;
> +      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");

Please don't add assertions that can trigger with
invalid/broken DWARF.  Call complaint instead.


> +		  if (TYPE_TYPEDEF_FIELD_PROTECTED (type, i))
> +		    {
> +		      if (section_type != s_protected)
> +			{
> +			  section_type = s_protected;
> +			  fprintfi_filtered (level + 2, stream,
> +					     "protected:\n");
> +			}
> +		    }
> +		  else if (TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> +		    {
> +		      if (section_type != s_private)
> +			{
> +			  section_type = s_private;
> +			  fprintfi_filtered (level + 2, stream,
> +					     "private:\n");
> +			}
> +		    }
> +		  else
> +		    {
> +		      gdb_assert (TYPE_TYPEDEF_FIELD_PUBLIC (type, i));

Won't this assertion fail with debug formats other than DWARF?
E.g., stabs?

> +		      if (section_type != s_public)
> +			{
> +			  section_type = s_public;
> +			  fprintfi_filtered (level + 2, stream,
> +					     "public:\n");
> +			}
> +		    }
> +


> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -884,6 +884,18 @@ struct typedef_field
>    /* * Type this typedef named NAME represents.  */
>  
>    struct type *type;
> +
> +  /* * True if this field was declared public, false otherwise.  */
> +  unsigned int is_public : 1;
> +
> +  /* * 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;
> +
> +  /* * Unused.  */
> +  unsigned int dummy : 13;

Is this really 13 bits?  Looks to me 29 bits on 32-bit archs (and due 
to padding, really 61 bits on 64-bit archs)?

>  };

For completeness, do you have a sense of whether this is a struct
that might have a significant impact on gdb's memory consumption?
Did you try measuring it with some large program, say, Firefox,
with -readnow ?

Thanks,
Pedro Alves