Re: [patch] Fix PR debug/82509

2017-10-19 Thread Richard Biener 
On Wed, Oct 18, 2017 at 3:54 PM, Eric Botcazou  wrote:
>> Hmm.  It makes tracking DIE builds difficult now that not all allocations go
>> through new_die anymore.
>
> I wouldn't have created such a precedent though, IOW there is nothing new.

Ah, didn't notice the other existing cases.

>> Can you instead split out a new_die_raw
>> function with just the allocation and the die_tag initialization?  Or make
>> !parent_die && !t this mode, thus add
>>
>>   if (parent_die != NULL)
>> add_child_die (parent_die, die);
>>   else if (! t)
>> return die;
>>   else
>>  {
>>
>> ?  Otherwise the patch looks sensible.
>
> Here's a revision version which makes sure that there is a single call to
>
>   ggc_cleared_alloc ()
>
> in the entire file.  Tested on x86_64-suse-linux.

Thanks, this is ok.

Richard.

>
> PR debug/82509
> * dwarf2out.c (new_die_raw): New static inline function.
> (new_die): Use it to create the DIE.
> (add_AT_external_die_ref): Likewise.
> (clone_die): Likewise.
> (clone_as_declaration): Likewise.
> (dwarf2out_vms_debug_main_pointer): Likewise.
> (base_type_die): Likewise.  Remove early return for corner cases.
> Do not call add_pubtype on the DIE here.
> (is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
> (modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
> typedefs for base types with DW_AT_endianity.  Make sure a DIE with
> native order exists for base types, attach the DIE manually and call
> add_pubtype on it.  Do not equate a reverse order DIE to the type.
>
> --
> Eric Botcazou

Re: [patch] Fix PR debug/82509

2017-10-18 Thread Eric Botcazou 
> Hmm.  It makes tracking DIE builds difficult now that not all allocations go
> through new_die anymore.

I wouldn't have created such a precedent though, IOW there is nothing new.

> Can you instead split out a new_die_raw
> function with just the allocation and the die_tag initialization?  Or make
> !parent_die && !t this mode, thus add
> 
>   if (parent_die != NULL)
> add_child_die (parent_die, die);
>   else if (! t)
> return die;
>   else
>  {
> 
> ?  Otherwise the patch looks sensible.

Here's a revision version which makes sure that there is a single call to

  ggc_cleared_alloc ()

in the entire file.  Tested on x86_64-suse-linux.


PR debug/82509
* dwarf2out.c (new_die_raw): New static inline function.
(new_die): Use it to create the DIE.
(add_AT_external_die_ref): Likewise.
(clone_die): Likewise.
(clone_as_declaration): Likewise.
(dwarf2out_vms_debug_main_pointer): Likewise.
(base_type_die): Likewise.  Remove early return for corner cases.
Do not call add_pubtype on the DIE here.
(is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
(modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
typedefs for base types with DW_AT_endianity.  Make sure a DIE with
native order exists for base types, attach the DIE manually and call
add_pubtype on it.  Do not equate a reverse order DIE to the type.

-- 
Eric BotcazouIndex: dwarf2out.c
===
--- dwarf2out.c	(revision 253848)
+++ dwarf2out.c	(working copy)
@@ -5364,6 +5364,16 @@ splice_child_die (dw_die_ref parent, dw_
   reparent_child (child, parent);
 }
 
+/* Create and return a new die with TAG_VALUE as tag.  */
+ 
+static inline dw_die_ref
+new_die_raw (enum dwarf_tag tag_value)
+{
+  dw_die_ref die = ggc_cleared_alloc ();
+  die->die_tag = tag_value;
+  return die;
+}
+
 /* Create and return a new die with a parent of PARENT_DIE.  If
PARENT_DIE is NULL, the new DIE is placed in limbo and an
associated tree T must be supplied to determine parenthood
@@ -5372,9 +5382,7 @@ splice_child_die (dw_die_ref parent, dw_
 static inline dw_die_ref
 new_die (enum dwarf_tag tag_value, dw_die_ref parent_die, tree t)
 {
-  dw_die_ref die = ggc_cleared_alloc ();
-
-  die->die_tag = tag_value;
+  dw_die_ref die = new_die_raw (tag_value);
 
   if (parent_die != NULL)
 add_child_die (parent_die, die);
@@ -5568,8 +5576,7 @@ add_AT_external_die_ref (dw_die_ref die,
 {
   /* Create a fake DIE that contains the reference.  Don't use
  new_die because we don't want to end up in the limbo list.  */
-  dw_die_ref ref = ggc_cleared_alloc ();
-  ref->die_tag = die->die_tag;
+  dw_die_ref ref = new_die_raw (die->die_tag);
   ref->die_id.die_symbol = IDENTIFIER_POINTER (get_identifier (symbol));
   ref->die_offset = offset;
   ref->with_offset = 1;
@@ -7712,13 +7719,10 @@ should_move_die_to_comdat (dw_die_ref di
 static dw_die_ref
 clone_die (dw_die_ref die)
 {
-  dw_die_ref clone;
+  dw_die_ref clone = new_die_raw (die->die_tag);
   dw_attr_node *a;
   unsigned ix;
 
-  clone = ggc_cleared_alloc ();
-  clone->die_tag = die->die_tag;
-
   FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
 add_dwarf_attr (clone, a);
 
@@ -7762,8 +7766,7 @@ clone_as_declaration (dw_die_ref die)
   return clone;
 }
 
-  clone = ggc_cleared_alloc ();
-  clone->die_tag = die->die_tag;
+  clone = new_die_raw (die->die_tag);
 
   FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
 {
@@ -12090,9 +12093,6 @@ base_type_die (tree type, bool reverse)
   struct fixed_point_type_info fpt_info;
   tree type_bias = NULL_TREE;
 
-  if (TREE_CODE (type) == ERROR_MARK || TREE_CODE (type) == VOID_TYPE)
-return 0;
-
   /* If this is a subtype that should not be emitted as a subrange type,
  use the base type.  See subrange_type_for_debug_p.  */
   if (TREE_CODE (type) == INTEGER_TYPE && TREE_TYPE (type) != NULL_TREE)
@@ -12185,7 +12185,7 @@ base_type_die (tree type, bool reverse)
   gcc_unreachable ();
 }
 
-  base_type_result = new_die (DW_TAG_base_type, comp_unit_die (), type);
+  base_type_result = new_die_raw (DW_TAG_base_type);
 
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
@@ -12241,8 +12241,6 @@ base_type_die (tree type, bool reverse)
 		 | dw_scalar_form_reference,
 		 NULL);
 
-  add_pubtype (type, base_type_result);
-
   return base_type_result;
 }
 
@@ -12270,8 +12268,6 @@ is_base_type (tree type)
 {
   switch (TREE_CODE (type))
 {
-case ERROR_MARK:
-case VOID_TYPE:
 case INTEGER_TYPE:
 case REAL_TYPE:
 case FIXED_POINT_TYPE:
@@ -12280,6 +12276,7 @@ is_base_type (tree type)
 case POINTER_BOUNDS_TYPE:
   return 1;
 
+case VOID_TYPE:
 case ARRAY_TYPE:
 case RECORD_TYPE:
 case UNION_TYPE:
@@ -12485,6 +12482,8 @@ modified_type_die (tree type, int cv_qua
   /* Only these

Re: [patch] Fix PR debug/82509

2017-10-18 Thread Richard Biener 
On Thu, Oct 12, 2017 at 10:51 PM, Eric Botcazou  wrote:
> Hi,
>
> this PR reports a couple of problems with the support of the DW_AT_endianity
> attribute associated with the scalar_storage_order source attribute: it does
> not persist through typedefs and it can contaminate native order DIEs.
>
> The attached patch revamps it by associating native order DIEs and reverse
> order DIEs into adjacent pairs for base types, as well as looking through
> typedefs for base types with reverse order.  This makes it possible to have a
> single reverse order DIE for each base type and look it up efficiently.
>
> Tested on x86_64-suse-linux, OK for the mainline?  What about the 7 branch?

Hmm.  It makes tracking DIE builds difficult now that not all allocations go
through new_die anymore.  Can you instead split out a new_die_raw
function with just the allocation and the die_tag initialization?  Or make
!parent_die && !t this mode, thus add

  if (parent_die != NULL)
add_child_die (parent_die, die);
  else if (! t)
return die;
  else
 {

?  Otherwise the patch looks sensible.

Thanks,
Richard.

>
> 2017-10-12  Eric Botcazou  
>
> PR debug/82509
> * dwarf2out.c (base_type_die): Remove early return for corner cases.
> Allocate the new DIE manually and do not call add_pubtype on it.
> (is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
> (modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
> typedefs for base types with DW_AT_endianity.  Make sure a DIE with
> native order exists for base types, attach the DIE manually and call
> add_pubtype on it.  Do not equate a reverse order DIE to the type.
>
>
> 2017-10-12  Eric Botcazou  
>
> * gcc.dg/debug/dwarf2/sso.c: Rename into...
> * gcc.dg/debug/dwarf2/sso-1.c: ...this.
> * gcc.dg/debug/dwarf2/sso-2.c: New test.
> * gcc.dg/debug/dwarf2/sso-3.c: Likewise.
>
> --
> Eric Botcazou

[patch] Fix PR debug/82509

2017-10-12 Thread Eric Botcazou 
Hi,

this PR reports a couple of problems with the support of the DW_AT_endianity 
attribute associated with the scalar_storage_order source attribute: it does 
not persist through typedefs and it can contaminate native order DIEs.

The attached patch revamps it by associating native order DIEs and reverse 
order DIEs into adjacent pairs for base types, as well as looking through 
typedefs for base types with reverse order.  This makes it possible to have a 
single reverse order DIE for each base type and look it up efficiently.

Tested on x86_64-suse-linux, OK for the mainline?  What about the 7 branch?


2017-10-12  Eric Botcazou  

PR debug/82509
* dwarf2out.c (base_type_die): Remove early return for corner cases.
Allocate the new DIE manually and do not call add_pubtype on it.
(is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
(modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
typedefs for base types with DW_AT_endianity.  Make sure a DIE with
native order exists for base types, attach the DIE manually and call
add_pubtype on it.  Do not equate a reverse order DIE to the type.


2017-10-12  Eric Botcazou  

* gcc.dg/debug/dwarf2/sso.c: Rename into...
* gcc.dg/debug/dwarf2/sso-1.c: ...this.
* gcc.dg/debug/dwarf2/sso-2.c: New test.
* gcc.dg/debug/dwarf2/sso-3.c: Likewise.

-- 
Eric BotcazouIndex: dwarf2out.c
===
--- dwarf2out.c	(revision 253628)
+++ dwarf2out.c	(working copy)
@@ -12090,9 +12090,6 @@ base_type_die (tree type, bool reverse)
   struct fixed_point_type_info fpt_info;
   tree type_bias = NULL_TREE;
 
-  if (TREE_CODE (type) == ERROR_MARK || TREE_CODE (type) == VOID_TYPE)
-return 0;
-
   /* If this is a subtype that should not be emitted as a subrange type,
  use the base type.  See subrange_type_for_debug_p.  */
   if (TREE_CODE (type) == INTEGER_TYPE && TREE_TYPE (type) != NULL_TREE)
@@ -12185,7 +12182,8 @@ base_type_die (tree type, bool reverse)
   gcc_unreachable ();
 }
 
-  base_type_result = new_die (DW_TAG_base_type, comp_unit_die (), type);
+  base_type_result = ggc_cleared_alloc ();
+  base_type_result->die_tag = DW_TAG_base_type;
 
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
@@ -12241,8 +12239,6 @@ base_type_die (tree type, bool reverse)
 		 | dw_scalar_form_reference,
 		 NULL);
 
-  add_pubtype (type, base_type_result);
-
   return base_type_result;
 }
 
@@ -12270,8 +12266,6 @@ is_base_type (tree type)
 {
   switch (TREE_CODE (type))
 {
-case ERROR_MARK:
-case VOID_TYPE:
 case INTEGER_TYPE:
 case REAL_TYPE:
 case FIXED_POINT_TYPE:
@@ -12280,6 +12274,7 @@ is_base_type (tree type)
 case POINTER_BOUNDS_TYPE:
   return 1;
 
+case VOID_TYPE:
 case ARRAY_TYPE:
 case RECORD_TYPE:
 case UNION_TYPE:
@@ -12485,6 +12480,8 @@ modified_type_die (tree type, int cv_qua
   /* Only these cv-qualifiers are currently handled.  */
   const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
 			| TYPE_QUAL_RESTRICT | TYPE_QUAL_ATOMIC);
+  const bool reverse_base_type
+= need_endianity_attribute_p (reverse) && is_base_type (type);
 
   if (code == ERROR_MARK)
 return NULL;
@@ -12535,29 +12532,33 @@ modified_type_die (tree type, int cv_qua
 	qualified_type = size_type_node;
 }
 
-
   /* If we do, then we can just use its DIE, if it exists.  */
   if (qualified_type)
 {
   mod_type_die = lookup_type_die (qualified_type);
 
-  /* DW_AT_endianity doesn't come from a qualifier on the type.  */
+  /* DW_AT_endianity doesn't come from a qualifier on the type, so it is
+	 dealt with specially: the DIE with the attribute, if it exists, is
+	 placed immediately after the regular DIE for the same base type.  */
   if (mod_type_die
-	  && (!need_endianity_attribute_p (reverse)
-	  || !is_base_type (type)
-	  || get_AT_unsigned (mod_type_die, DW_AT_endianity)))
+	  && (!reverse_base_type
+	  || ((mod_type_die = mod_type_die->die_sib) != NULL
+		  && get_AT_unsigned (mod_type_die, DW_AT_endianity
 	return mod_type_die;
 }
 
   name = qualified_type ? TYPE_NAME (qualified_type) : NULL;
 
   /* Handle C typedef types.  */
-  if (name && TREE_CODE (name) == TYPE_DECL && DECL_ORIGINAL_TYPE (name)
+  if (name
+  && TREE_CODE (name) == TYPE_DECL
+  && DECL_ORIGINAL_TYPE (name)
   && !DECL_ARTIFICIAL (name))
 {
   tree dtype = TREE_TYPE (name);
 
-  if (qualified_type == dtype)
+  /* Skip the typedef for base types with DW_AT_endianity, no big deal.  */
+  if (qualified_type == dtype && !reverse_base_type)
 	{
 	  tree origin = decl_ultimate_origin (name);
 
@@ -12729,7 +12730,21 @@ modified_type_die (tree type, int cv_qua
   item_type = TREE_TYPE (type);
 }
   else if