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 cv-qua

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