| Message ID | 14641386.x2jigZTk3C@polaris |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR debug/82509 | expand |
On Thu, Oct 12, 2017 at 10:51 PM, Eric Botcazou <ebotcazou@adacore.com> 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 <ebotcazou@adacore.com> > > 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 <ebotcazou@adacore.com> > > * 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
> 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<die_node> () 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.
On Wed, Oct 18, 2017 at 3:54 PM, Eric Botcazou <ebotcazou@adacore.com> 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<die_node> () > > 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
Index: 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<die_node> (); + 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 (is_base_type (type)) - mod_type_die = base_type_die (type, reverse); + { + mod_type_die = base_type_die (type, reverse); + + /* The DIE with DW_AT_endianity is placed right after the naked DIE. */ + if (reverse_base_type) + { + dw_die_ref after_die + = modified_type_die (type, cv_quals, false, context_die); + add_child_die_after (comp_unit_die (), mod_type_die, after_die); + } + else + add_child_die (comp_unit_die (), mod_type_die); + + add_pubtype (type, mod_type_die); + } else { gen_type_die (type, context_die); @@ -12791,7 +12806,7 @@ modified_type_die (tree type, int cv_qua name ? IDENTIFIER_POINTER (name) : "__unknown__"); } - if (qualified_type) + if (qualified_type && !reverse_base_type) equate_type_number_to_die (qualified_type, mod_type_die); if (item_type)