Patchwork Fix up DW_AT_accessibility (PR debug/45124)

login
register
mail settings
Submitter Jakub Jelinek
Date July 29, 2010, 8:20 p.m.
Message ID <20100729202053.GO18378@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/60312/
State New
Headers show

Comments

Jakub Jelinek - July 29, 2010, 8:20 p.m.
On Thu, Jul 29, 2010 at 01:13:25PM -0400, Jason Merrill wrote:
> OK.

Actually, given the response on Dwarf-Discuss, I believe we should
limit this only to DW_TAG_member and DW_TAG_subprogram (to match the spec;
DW_TAG_inheritance handled in the other routine), as for say types and other
children of DW_TAG_class_type no DECL_PRIVATE doesn't mean we need to emit
DW_AT_accessibility DW_ACCESS_public, but instead just that accessibility
is not meaningful for it.

2010-07-29  Jakub Jelinek  <jakub@redhat.com>

	PR debug/45124
	* dwarf2out.c (add_accessibility_attribute): Assume
	DW_ACCESS_private as the default for dwarf_version > 2
	and DW_TAG_class_type parent.
	(gen_inheritance_die): Assume DW_ACCESS_public as the default
	for dwarf_version > 2 and parent other than DW_TAG_class_type.



	Jakub
Richard Henderson - July 30, 2010, 6:33 p.m.
On 07/29/2010 01:20 PM, Jakub Jelinek wrote:
> +  /* In DWARF3+ the default is DW_ACCESS_private only in DW_TAG_class_type
> +     children, otherwise the default is DW_ACCESS_public.  In DWARF2
> +     the default has always been DW_ACCESS_public.  */
>    if (TREE_PROTECTED (decl))
>      add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_protected);
>    else if (TREE_PRIVATE (decl))
> -    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
> +    {
> +      if (dwarf_version == 2
> +	  || die->die_parent == NULL
> +	  || die->die_parent->die_tag != DW_TAG_class_type
> +	  || (die->die_tag != DW_TAG_member
> +	      && die->die_tag != DW_TAG_subprogram))
> +	add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
> +    }
> +  else if (dwarf_version > 2
> +	   && die->die_parent
> +	   && die->die_parent->die_tag == DW_TAG_class_type
> +	   && (die->die_tag == DW_TAG_member
> +	       || die->die_tag == DW_TAG_subprogram))
> +    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_public);

Your code matches the comment, but the comment is wrong: DWARF2 default is private.

You said it correctly in the email, but not in the code.



r~
Jakub Jelinek - July 30, 2010, 6:43 p.m.
On Fri, Jul 30, 2010 at 11:33:51AM -0700, Richard Henderson wrote:
> On 07/29/2010 01:20 PM, Jakub Jelinek wrote:
> > +  /* In DWARF3+ the default is DW_ACCESS_private only in DW_TAG_class_type
> > +     children, otherwise the default is DW_ACCESS_public.  In DWARF2
> > +     the default has always been DW_ACCESS_public.  */
> >    if (TREE_PROTECTED (decl))
> >      add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_protected);
> >    else if (TREE_PRIVATE (decl))
> > -    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
> > +    {
> > +      if (dwarf_version == 2
> > +	  || die->die_parent == NULL
> > +	  || die->die_parent->die_tag != DW_TAG_class_type
> > +	  || (die->die_tag != DW_TAG_member
> > +	      && die->die_tag != DW_TAG_subprogram))
> > +	add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
> > +    }
> > +  else if (dwarf_version > 2
> > +	   && die->die_parent
> > +	   && die->die_parent->die_tag == DW_TAG_class_type
> > +	   && (die->die_tag == DW_TAG_member
> > +	       || die->die_tag == DW_TAG_subprogram))
> > +    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_public);
> 
> Your code matches the comment, but the comment is wrong: DWARF2 default is private.

DWARF2 default for DW_TAG_inheritance if DW_AT_accessibility is missing is
private, yes (and the other hunk which handles DW_TAG_inheritance implements
that).
DWARF2 doesn't seem to talk at all about default for DW_TAG_member or
DW_TAG_subprogram, so the patch above just kept status quo in that case for
DWARF2 and only changed to what DWARF3 says for dwarf_version > 2.
Other options are emit DW_AT_accessibility always for
DW_TAG_member/DW_TAG_subprogram for dwarf_version == 2, or pretend the
DWARF3+ rules (private in DW_TAG_class_type, public otherwise) apply even
for DWARF2.  Not sure what is best.

	Jakub
Jason Merrill - Sept. 17, 2010, 2:05 p.m.
On 07/29/2010 04:20 PM, Jakub Jelinek wrote:
> Actually, given the response on Dwarf-Discuss, I believe we should
> limit this only to DW_TAG_member and DW_TAG_subprogram (to match the spec;
> DW_TAG_inheritance handled in the other routine), as for say types and other
> children of DW_TAG_class_type no DECL_PRIVATE doesn't mean we need to emit
> DW_AT_accessibility DW_ACCESS_public, but instead just that accessibility
> is not meaningful for it.

All class members have accessibility; the accessibility of a nested type 
or enumerator has the same meaning as for a data or function member.

> DWARF2 default for DW_TAG_inheritance if DW_AT_accessibility is missing is
> private, yes (and the other hunk which handles DW_TAG_inheritance implements
> that).
> DWARF2 doesn't seem to talk at all about default for DW_TAG_member or
> DW_TAG_subprogram, so the patch above just kept status quo in that case for
> DWARF2 and only changed to what DWARF3 says for dwarf_version > 2.
> Other options are emit DW_AT_accessibility always for
> DW_TAG_member/DW_TAG_subprogram for dwarf_version == 2, or pretend the
> DWARF3+ rules (private in DW_TAG_class_type, public otherwise) apply even
> for DWARF2.  Not sure what is best.

Sticking with the status quo for DWARF2 is fine.

Jason
Jakub Jelinek - Sept. 17, 2010, 5:44 p.m.
On Fri, Sep 17, 2010 at 10:05:18AM -0400, Jason Merrill wrote:
> On 07/29/2010 04:20 PM, Jakub Jelinek wrote:
> >Actually, given the response on Dwarf-Discuss, I believe we should
> >limit this only to DW_TAG_member and DW_TAG_subprogram (to match the spec;
> >DW_TAG_inheritance handled in the other routine), as for say types and other
> >children of DW_TAG_class_type no DECL_PRIVATE doesn't mean we need to emit
> >DW_AT_accessibility DW_ACCESS_public, but instead just that accessibility
> >is not meaningful for it.
> 
> All class members have accessibility; the accessibility of a nested
> type or enumerator has the same meaning as for a data or function
> member.

So, should GCC assume for DWARF3+ that the default is always what the
language (C++ in this case) says is the default (and adjust the standard)?

> >DWARF2 default for DW_TAG_inheritance if DW_AT_accessibility is missing is
> >private, yes (and the other hunk which handles DW_TAG_inheritance implements
> >that).
> >DWARF2 doesn't seem to talk at all about default for DW_TAG_member or
> >DW_TAG_subprogram, so the patch above just kept status quo in that case for
> >DWARF2 and only changed to what DWARF3 says for dwarf_version > 2.
> >Other options are emit DW_AT_accessibility always for
> >DW_TAG_member/DW_TAG_subprogram for dwarf_version == 2, or pretend the
> >DWARF3+ rules (private in DW_TAG_class_type, public otherwise) apply even
> >for DWARF2.  Not sure what is best.
> 
> Sticking with the status quo for DWARF2 is fine.

Ok.

	Jakub
class A
{
  typedef int AD1;
  enum AD2 { ad21, ad22 };
  struct AD3 { typedef int C1; C1 i; };
  class AD4 { typedef int C2; C2 i; };
  int AD5;
  static int AD6;
  int AD7 () {}
  static int AD8 () {}
private:
  typedef int AP1;
  enum AP2 { ap21, ap22 };
  struct AP3 { typedef int C3; C3 i; };
  class AP4 { typedef int C4; C4 i; };
  int AP5;
  static int AP6;
  int AP7 () {}
  static int AP8 () {}
protected:
  typedef int AR1;
  enum AR2 { ar21, ar22 };
  struct AR3 { typedef int C5; C5 i; };
  class AR4 { typedef int C6; C6 i; };
  int AR5;
  static int AR6;
  int AR7 () {}
  static int AR8 () {}
public:
  typedef int AU1;
  enum AU2 { au21, au22 };
  struct AU3 { typedef int C7; C7 i; };
  class AU4 { typedef int C8; C8 i; };
  int AU5;
  static int AU6;
  int AU7 () {}
  static int AU8 () {}
public:
  AD1 ad1; AD2 ad2; AP1 ap1; AP2 ap2; AR1 ar1; AR2 ar2; AU1 au1; AU2 au2;
  AD3 ad3; AD4 ad4; AP3 ap3; AP4 ap4; AR3 ar3; AR4 ar4; AU3 au3; AU4 au4;
};
struct B
{
  typedef int BD1;
  enum BD2 { bd21, bd22 };
  struct BD3 { typedef int C9; C9 i; };
  class BD4 { typedef int C10; C10 i; };
  int BD5;
  static int BD6;
  int BD7 () {}
  static int BD8 () {}
private:
  typedef int BP1;
  enum BP2 { bp21, bp22 };
  struct BP3 { typedef int C11; C11 i; };
  class BP4 { typedef int C12; C12 i; };
  int BP5;
  static int BP6;
  int BP7 () {}
  static int BP8 () {}
protected:
  typedef int BR1;
  enum BR2 { br21, br22 };
  struct BR3 { typedef int C13; C13 i; };
  class BR4 { typedef int C14; C14 i; };
  int BR5;
  static int BR6;
  int BR7 () {}
  static int BR8 () {}
public:
  typedef int BU1;
  enum BU2 { bu21, bu22 };
  struct BU3 { typedef int C15; C15 i; };
  class BU4 { typedef int C16; C16 i; };
  int BU5;
  static int BU6;
  int BU7 () {}
  static int BU8 () {}
public:
  BD1 bd1; BD2 bd2; BP1 bp1; BP2 bp2; BR1 br1; BR2 br2; BU1 bu1; BU2 bu2;
  BD3 bd3; BD4 bd4; BP3 bp3; BP4 bp4; BR3 br3; BR4 br4; BU3 bu3; BU4 bu4;
};
A a;
B b;
Jason Merrill - Sept. 17, 2010, 6:17 p.m.
On 09/17/2010 01:44 PM, Jakub Jelinek wrote:
> So, should GCC assume for DWARF3+ that the default is always what the
> language (C++ in this case) says is the default (and adjust the standard)?

Yes.

Jason

Patch

--- gcc/dwarf2out.c.jj	2010-07-29 21:49:03.827604473 +0200
+++ gcc/dwarf2out.c	2010-07-29 22:14:28.231354703 +0200
@@ -15834,10 +15834,26 @@  add_AT_location_description (dw_die_ref 
 static void
 add_accessibility_attribute (dw_die_ref die, tree decl)
 {
+  /* In DWARF3+ the default is DW_ACCESS_private only in DW_TAG_class_type
+     children, otherwise the default is DW_ACCESS_public.  In DWARF2
+     the default has always been DW_ACCESS_public.  */
   if (TREE_PROTECTED (decl))
     add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_protected);
   else if (TREE_PRIVATE (decl))
-    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
+    {
+      if (dwarf_version == 2
+	  || die->die_parent == NULL
+	  || die->die_parent->die_tag != DW_TAG_class_type
+	  || (die->die_tag != DW_TAG_member
+	      && die->die_tag != DW_TAG_subprogram))
+	add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
+    }
+  else if (dwarf_version > 2
+	   && die->die_parent
+	   && die->die_parent->die_tag == DW_TAG_class_type
+	   && (die->die_tag == DW_TAG_member
+	       || die->die_tag == DW_TAG_subprogram))
+    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_public);
 }
 
 /* Attach the specialized form of location attribute used for data members of
@@ -19567,10 +19583,20 @@  gen_inheritance_die (tree binfo, tree ac
   if (BINFO_VIRTUAL_P (binfo))
     add_AT_unsigned (die, DW_AT_virtuality, DW_VIRTUALITY_virtual);
 
+  /* In DWARF3+ the default is DW_ACCESS_private only in DW_TAG_class_type
+     children, otherwise the default is DW_ACCESS_public.  In DWARF2
+     the default has always been DW_ACCESS_private.  */
   if (access == access_public_node)
-    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_public);
+    {
+      if (dwarf_version == 2
+	  || context_die->die_tag == DW_TAG_class_type)
+      add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_public);
+    }
   else if (access == access_protected_node)
     add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_protected);
+  else if (dwarf_version > 2
+	   && context_die->die_tag != DW_TAG_class_type)
+    add_AT_unsigned (die, DW_AT_accessibility, DW_ACCESS_private);
 }
 
 /* Generate a DIE for a class member.  */