diff mbox series

Fix debug info for enumeration types with reverse Scalar_Storage_Order

Message ID 3238326.AJdgDx1Vlc@fomalhaut
State Accepted
Headers
Series Fix debug info for enumeration types with reverse Scalar_Storage_Order |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Eric Botcazou Jan. 9, 2024, 8:17 p.m. UTC 
  Hi,

this is not really a regression but the patch was written last week and is 
quite straightforward, so hopefully can nevertheless be OK.  It implements the 
support of DW_AT_endianity for enumeration types because they are scalar and, 
therefore, reverse Scalar_Storage_Order is supported for them, but only when 
the -gstrict-dwarf switch is not passed because this is an extension.

There is an associated GDB patch to be submitted by Tom to grok the new DWARF.

Tested on x86-64/Linux, OK for the mainline?  It may also help the GDB side to 
backport it for the upcoming 13.3 release.


2024-01-09  Eric Botcazou  <ebotcazou@adacore.com>

	* dwarf2out.cc (modified_type_die): Extend the support of reverse
	storage order to enumeration types if -gstrict-dwarf is not passed.
	(gen_enumeration_type_die): Add REVERSE parameter and generate the
 	DIE immediately after the existing one if it is true.
	(gen_tagged_type_die): Add REVERSE parameter and pass it in the
	call to gen_enumeration_type_die.
	(gen_type_die_with_usage): Add REVERSE parameter and pass it in the
	first recursive call as well as the call to gen_tagged_type_die.
	(gen_type_die): Add REVERSE parameter and pass it in the call to
	gen_type_die_with_usage.

Comments

Richard Biener Jan. 10, 2024, 9:37 a.m. UTC | #1 
On Tue, Jan 9, 2024 at 9:18 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> Hi,
>
> this is not really a regression but the patch was written last week and is
> quite straightforward, so hopefully can nevertheless be OK.  It implements the
> support of DW_AT_endianity for enumeration types because they are scalar and,
> therefore, reverse Scalar_Storage_Order is supported for them, but only when
> the -gstrict-dwarf switch is not passed because this is an extension.
>
> There is an associated GDB patch to be submitted by Tom to grok the new DWARF.
>
> Tested on x86-64/Linux, OK for the mainline?  It may also help the GDB side to
> backport it for the upcoming 13.3 release.

Can you elaborate on the DIE order constraint and why it was chosen?  That is,

+      /* The DIE with DW_AT_endianity is placed right after the naked DIE.  */
+      if (reverse)
+       {
+         gcc_assert (type_die);
...

and

+      /* The DIE with DW_AT_endianity is placed right after the naked DIE.  */
+      if (reverse_type)
+       {
+         dw_die_ref after_die
+           = modified_type_die (type, cv_quals, false, context_die);
+         gen_type_die (type, context_die, true);
+         gcc_assert (after_die->die_sib
+                     && get_AT_unsigned (after_die->die_sib, DW_AT_endianity));
+         return after_die->die_sib;

?

Likewise the extra argument to the functions is odd - is that not available
on the tree type?

Richard.

>
> 2024-01-09  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * dwarf2out.cc (modified_type_die): Extend the support of reverse
>         storage order to enumeration types if -gstrict-dwarf is not passed.
>         (gen_enumeration_type_die): Add REVERSE parameter and generate the
>         DIE immediately after the existing one if it is true.
>         (gen_tagged_type_die): Add REVERSE parameter and pass it in the
>         call to gen_enumeration_type_die.
>         (gen_type_die_with_usage): Add REVERSE parameter and pass it in the
>         first recursive call as well as the call to gen_tagged_type_die.
>         (gen_type_die): Add REVERSE parameter and pass it in the call to
>         gen_type_die_with_usage.
>
> --
> Eric Botcazou
Eric Botcazou Jan. 10, 2024, 11:53 a.m. UTC | #2 
> Can you elaborate on the DIE order constraint and why it was chosen?  That
> is,
> 
> +      /* The DIE with DW_AT_endianity is placed right after the naked DIE. 
> */ +      if (reverse)
> +       {
> +         gcc_assert (type_die);
> ...
> 
> and
> 
> +      /* The DIE with DW_AT_endianity is placed right after the naked DIE. 
> */ +      if (reverse_type)
> +       {
> +         dw_die_ref after_die
> +           = modified_type_die (type, cv_quals, false, context_die);
> +         gen_type_die (type, context_die, true);
> +         gcc_assert (after_die->die_sib
> +                     && get_AT_unsigned (after_die->die_sib,
> DW_AT_endianity)); +         return after_die->die_sib;
> 
> ?

That's preexisting though, see line 13730 where there is a small blurb.

The crux of the matter is that there is no scalar *_TYPE node with a reverse 
SSO, so there is nothing to equate with for the DIE carrying DW_AT_endianity, 
unlike for type variants (the reverse SSO is on the enclosing aggregate type 
instead but this does not match the way DWARF describes it).

Therefore, in order to avoid building a new DIE with DW_AT_endianity each 
time, the DIE with DW_AT_endianity is placed right after the naked DIE, so 
that the lookup done at line 13730 for reverse SSO is immediate.

> Likewise the extra argument to the functions is odd - is that not available
> on the tree type?

No, for the reason described above, so the extra parameter is preexisting for 
base_type_die, modified_type_die and add_type_attribute.
Richard Biener Jan. 10, 2024, 11:54 a.m. UTC | #3 
On Wed, Jan 10, 2024 at 12:53 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Can you elaborate on the DIE order constraint and why it was chosen?  That
> > is,
> >
> > +      /* The DIE with DW_AT_endianity is placed right after the naked DIE.
> > */ +      if (reverse)
> > +       {
> > +         gcc_assert (type_die);
> > ...
> >
> > and
> >
> > +      /* The DIE with DW_AT_endianity is placed right after the naked DIE.
> > */ +      if (reverse_type)
> > +       {
> > +         dw_die_ref after_die
> > +           = modified_type_die (type, cv_quals, false, context_die);
> > +         gen_type_die (type, context_die, true);
> > +         gcc_assert (after_die->die_sib
> > +                     && get_AT_unsigned (after_die->die_sib,
> > DW_AT_endianity)); +         return after_die->die_sib;
> >
> > ?
>
> That's preexisting though, see line 13730 where there is a small blurb.
>
> The crux of the matter is that there is no scalar *_TYPE node with a reverse
> SSO, so there is nothing to equate with for the DIE carrying DW_AT_endianity,
> unlike for type variants (the reverse SSO is on the enclosing aggregate type
> instead but this does not match the way DWARF describes it).
>
> Therefore, in order to avoid building a new DIE with DW_AT_endianity each
> time, the DIE with DW_AT_endianity is placed right after the naked DIE, so
> that the lookup done at line 13730 for reverse SSO is immediate.

Hmm, I see.  The patch is OK then.

Thanks,
Richard.

> > Likewise the extra argument to the functions is odd - is that not available
> > on the tree type?
>
> No, for the reason described above, so the extra parameter is preexisting for
> base_type_die, modified_type_die and add_type_attribute.
>
> --
> Eric Botcazou
>
>
diff mbox series

Patch

diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 2f9010bc3cb..1c994bb8b9b 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -3940,7 +3940,7 @@  static void gen_descr_array_type_die (tree, struct array_descr_info *, dw_die_re
 #if 0
 static void gen_entry_point_die (tree, dw_die_ref);
 #endif
-static dw_die_ref gen_enumeration_type_die (tree, dw_die_ref);
+static dw_die_ref gen_enumeration_type_die (tree, dw_die_ref, bool);
 static dw_die_ref gen_formal_parameter_die (tree, tree, bool, dw_die_ref);
 static dw_die_ref gen_formal_parameter_pack_die  (tree, tree, dw_die_ref, tree*);
 static void gen_unspecified_parameters_die (tree, dw_die_ref);
@@ -3960,7 +3960,7 @@  static void gen_struct_or_union_type_die (tree, dw_die_ref,
 						enum debug_info_usage);
 static void gen_subroutine_type_die (tree, dw_die_ref);
 static void gen_typedef_die (tree, dw_die_ref);
-static void gen_type_die (tree, dw_die_ref);
+static void gen_type_die (tree, dw_die_ref, bool = false);
 static void gen_block_die (tree, dw_die_ref);
 static void decls_for_scope (tree, dw_die_ref, bool = true);
 static bool is_naming_typedef_decl (const_tree);
@@ -3976,8 +3976,10 @@  static struct dwarf_file_data * lookup_filename (const char *);
 static void retry_incomplete_types (void);
 static void gen_type_die_for_member (tree, tree, dw_die_ref);
 static void gen_generic_params_dies (tree);
-static void gen_tagged_type_die (tree, dw_die_ref, enum debug_info_usage);
-static void gen_type_die_with_usage (tree, dw_die_ref, enum debug_info_usage);
+static void gen_tagged_type_die (tree, dw_die_ref, enum debug_info_usage,
+				 bool = false);
+static void gen_type_die_with_usage (tree, dw_die_ref, enum debug_info_usage,
+				     bool = false);
 static void splice_child_die (dw_die_ref, dw_die_ref);
 static int file_info_cmp (const void *, const void *);
 static dw_loc_list_ref new_loc_list (dw_loc_descr_ref, const char *, var_loc_view,
@@ -13665,8 +13667,11 @@  modified_type_die (tree type, int cv_quals, bool reverse,
   const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
 			    | TYPE_QUAL_RESTRICT | TYPE_QUAL_ATOMIC | 
 			    ENCODE_QUAL_ADDR_SPACE(~0U));
-  const bool reverse_base_type
-    = need_endianity_attribute_p (reverse) && is_base_type (type);
+  /* DW_AT_endianity is specified only for base types in the standard.  */
+  const bool reverse_type
+    = need_endianity_attribute_p (reverse)
+      && (is_base_type (type)
+	  || (TREE_CODE (type) == ENUMERAL_TYPE && !dwarf_strict));
 
   if (code == ERROR_MARK)
     return NULL;
@@ -13726,9 +13731,9 @@  modified_type_die (tree type, int cv_quals, bool reverse,
 
       /* 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.  */
+	 placed immediately after the regular DIE for the same type.  */
       if (mod_type_die
-	  && (!reverse_base_type
+	  && (!reverse_type
 	      || ((mod_type_die = mod_type_die->die_sib) != NULL
 		  && get_AT_unsigned (mod_type_die, DW_AT_endianity))))
 	return mod_type_die;
@@ -13745,7 +13750,7 @@  modified_type_die (tree type, int cv_quals, bool reverse,
       tree dtype = TREE_TYPE (name);
 
       /* Skip the typedef for base types with DW_AT_endianity, no big deal.  */
-      if (qualified_type == dtype && !reverse_base_type)
+      if (qualified_type == dtype && !reverse_type)
 	{
 	  tree origin = decl_ultimate_origin (name);
 
@@ -13952,7 +13957,7 @@  modified_type_die (tree type, int cv_quals, bool 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)
+      if (reverse_type)
 	{
 	  dw_die_ref after_die
 	    = modified_type_die (type, cv_quals, false, context_die);
@@ -13965,6 +13970,17 @@  modified_type_die (tree type, int cv_quals, bool reverse,
     }
   else
     {
+      /* The DIE with DW_AT_endianity is placed right after the naked DIE.  */
+      if (reverse_type)
+	{
+	  dw_die_ref after_die
+	    = modified_type_die (type, cv_quals, false, context_die);
+	  gen_type_die (type, context_die, true);
+	  gcc_assert (after_die->die_sib
+		      && get_AT_unsigned (after_die->die_sib, DW_AT_endianity));
+	  return after_die->die_sib;
+	}
+
       gen_type_die (type, context_die);
 
       /* We have to get the type_main_variant here (and pass that to the
@@ -14034,7 +14050,7 @@  modified_type_die (tree type, int cv_quals, bool reverse,
 	}
     }
 
-  if (qualified_type && !reverse_base_type)
+  if (qualified_type && !reverse_type)
     equate_type_number_to_die (qualified_type, mod_type_die);
 
   if (item_type)
@@ -22824,19 +22840,31 @@  record_type_tag (tree type)
 /* Generate a DIE to represent an enumeration type.  Note that these DIEs
    include all of the information about the enumeration values also. Each
    enumerated type name/value is listed as a child of the enumerated type
-   DIE.  */
+   DIE. REVERSE is true if the type is to be interpreted in the reverse
+   storage order wrt the target order.  */
 
 static dw_die_ref
-gen_enumeration_type_die (tree type, dw_die_ref context_die)
+gen_enumeration_type_die (tree type, dw_die_ref context_die, bool reverse)
 {
   dw_die_ref type_die = lookup_type_die (type);
   dw_die_ref orig_type_die = type_die;
 
-  if (type_die == NULL)
+  if (type_die == NULL || reverse)
     {
-      type_die = new_die (DW_TAG_enumeration_type,
-			  scope_die_for (type, context_die), type);
-      equate_type_number_to_die (type, type_die);
+      /* The DIE with DW_AT_endianity is placed right after the naked DIE.  */
+      if (reverse)
+	{
+	  gcc_assert (type_die);
+	  dw_die_ref after_die = type_die;
+	  type_die = new_die_raw (DW_TAG_enumeration_type);
+	  add_child_die_after (context_die, type_die, after_die);
+	}
+      else
+	{
+	  type_die = new_die (DW_TAG_enumeration_type,
+			      scope_die_for (type, context_die), type);
+	  equate_type_number_to_die (type, type_die);
+	}
       add_name_attribute (type_die, type_tag (type));
       if ((dwarf_version >= 4 || !dwarf_strict)
 	  && ENUM_IS_SCOPED (type))
@@ -22848,6 +22876,9 @@  gen_enumeration_type_die (tree type, dw_die_ref context_die)
 			 TYPE_UNSIGNED (type)
 			 ? DW_ATE_unsigned
 			 : DW_ATE_signed);
+      if (reverse)
+	add_AT_unsigned (type_die, DW_AT_endianity,
+			 BYTES_BIG_ENDIAN ? DW_END_little : DW_END_big);
     }
   else if (! TYPE_SIZE (type) || ENUM_IS_OPAQUE (type))
     return type_die;
@@ -26155,7 +26186,8 @@  gen_typedef_die (tree decl, dw_die_ref context_die)
 static void
 gen_tagged_type_die (tree type,
 		     dw_die_ref context_die,
-		     enum debug_info_usage usage)
+		     enum debug_info_usage usage,
+		     bool reverse)
 {
   if (type == NULL_TREE
       || !is_tagged_type (type))
@@ -26200,8 +26232,8 @@  gen_tagged_type_die (tree type,
     {
       /* This might have been written out by the call to
 	 declare_in_namespace.  */
-      if (!TREE_ASM_WRITTEN (type))
-	gen_enumeration_type_die (type, context_die);
+      if (!TREE_ASM_WRITTEN (type) || reverse)
+	gen_enumeration_type_die (type, context_die, reverse);
     }
   else
     gen_struct_or_union_type_die (type, context_die, usage);
@@ -26215,7 +26247,7 @@  gen_tagged_type_die (tree type,
 
 static void
 gen_type_die_with_usage (tree type, dw_die_ref context_die,
-			 enum debug_info_usage usage)
+			 enum debug_info_usage usage, bool reverse)
 {
   struct array_descr_info info;
 
@@ -26279,7 +26311,7 @@  gen_type_die_with_usage (tree type, dw_die_ref context_die,
 
       if (debug_type != NULL_TREE && debug_type != type)
 	{
-	  gen_type_die_with_usage (debug_type, context_die, usage);
+	  gen_type_die_with_usage (debug_type, context_die, usage, reverse);
 	  return;
 	}
     }
@@ -26326,7 +26358,7 @@  gen_type_die_with_usage (tree type, dw_die_ref context_die,
 	}
     }
 
-  if (TREE_ASM_WRITTEN (type))
+  if (TREE_ASM_WRITTEN (type) && !reverse)
     {
       /* Variable-length types may be incomplete even if
 	 TREE_ASM_WRITTEN.  For such types, fall through to
@@ -26398,7 +26430,7 @@  gen_type_die_with_usage (tree type, dw_die_ref context_die,
     case RECORD_TYPE:
     case UNION_TYPE:
     case QUAL_UNION_TYPE:
-      gen_tagged_type_die (type, context_die, usage);
+      gen_tagged_type_die (type, context_die, usage, reverse);
       return;
 
     case VOID_TYPE:
@@ -26450,11 +26482,11 @@  gen_type_die_with_usage (tree type, dw_die_ref context_die,
 }
 
 static void
-gen_type_die (tree type, dw_die_ref context_die)
+gen_type_die (tree type, dw_die_ref context_die, bool reverse)
 {
   if (type != error_mark_node)
     {
-      gen_type_die_with_usage (type, context_die, DINFO_USAGE_DIR_USE);
+      gen_type_die_with_usage (type, context_die, DINFO_USAGE_DIR_USE, reverse);
       if (flag_checking)
 	{
 	  dw_die_ref die = lookup_type_die (type);