Re: [PATCH] Fix scoped enum debug info (PR debug/58150)

[Jason Merrill]

OK.

On Fri, Mar 9, 2018 at 1:15 PM, Jakub Jelinek  wrote:
> Hi!
>
> As the following testcase shows, we emit bad debug info if a scoped
> enum is used before the enumerators are defined.
> gen_enumeration_type_die has support for enum forward declarations that
> have NULL TYPE_SIZE (i.e. incomplete), but in this case it is complete,
> just is ENUM_IS_OPAQUE and the enumerators aren't there.  We still set
> TREE_ASM_WRITTEN on it and therefore don't do anything when it actually is
> completed.
>
> The following patch does change/fix a bunch of things:
> 1) I don't see the point of guarding the addition of DW_AT_declaration
> to just -gdwarf-4 or -gno-strict-dwarf, that is already DWARF2 attribute and
> we are emitting it for the forward declarations already
> 2) we don't set TREE_ASM_WRITTEN if ENUM_IS_OPAQUE
> 3) we don't try to do anything if it is ENUM_IS_OPAQUE that hasn't been
> newly created; similarly to incomplete forward redeclarations, we should
> have provided all that is needed on the first declaration
> 4) the addition of most attributes guarded with TYPE_SIZE is guarded by
> (!original_type_die || !get_AT (type_die, DW_AT_...)); the
> !original_type_die || part is just an optimization, so we don't try to query
> it in the common cases where we are creating a fresh type die; anyway, the
> reason for the guards is that we can now do the code twice for the same
> type_die (on declaration and definition), and don't want to add the attributes
> twice
> 5) not sure if ENUM_IS_OPAQUE must always imply non-NULL TYPE_SIZE, if not,
> in that case we'd add the DW_AT_deprecated attribute twice
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-03-09  Jakub Jelinek  
>
> PR debug/58150
> * dwarf2out.c (gen_enumeration_type_die): Don't guard adding
> DW_AT_declaration for ENUM_IS_OPAQUE on -gdwarf-4 or 
> -gno-strict-dwarf,
> but on TYPE_SIZE.  Don't do anything for ENUM_IS_OPAQUE if not 
> creating
> a new die.  Don't set TREE_ASM_WRITTEN if ENUM_IS_OPAQUE.  Guard
> addition of most attributes on !orig_type_die or the attribute not
> being present already.  Assert TYPE_VALUES is NULL for ENUM_IS_OPAQUE.
>
> * g++.dg/debug/dwarf2/enum2.C: New test.
>
> --- gcc/dwarf2out.c.jj  2018-03-08 22:49:41.965225512 +0100
> +++ gcc/dwarf2out.c 2018-03-09 14:53:15.596115625 +0100
> @@ -21839,6 +21839,7 @@ static dw_die_ref
>  gen_enumeration_type_die (tree type, dw_die_ref context_die)
>  {
>dw_die_ref type_die = lookup_type_die (type);
> +  dw_die_ref orig_type_die = type_die;
>
>if (type_die == NULL)
>  {
> @@ -21846,20 +21847,18 @@ gen_enumeration_type_die (tree type, dw_
>   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)
> -   {
> - if (ENUM_IS_SCOPED (type))
> -   add_AT_flag (type_die, DW_AT_enum_class, 1);
> - if (ENUM_IS_OPAQUE (type))
> -   add_AT_flag (type_die, DW_AT_declaration, 1);
> -   }
> +  if ((dwarf_version >= 4 || !dwarf_strict)
> + && ENUM_IS_SCOPED (type))
> +   add_AT_flag (type_die, DW_AT_enum_class, 1);
> +  if (ENUM_IS_OPAQUE (type) && TYPE_SIZE (type))
> +   add_AT_flag (type_die, DW_AT_declaration, 1);
>if (!dwarf_strict)
> add_AT_unsigned (type_die, DW_AT_encoding,
>  TYPE_UNSIGNED (type)
>  ? DW_ATE_unsigned
>  : DW_ATE_signed);
>  }
> -  else if (! TYPE_SIZE (type))
> +  else if (! TYPE_SIZE (type) || ENUM_IS_OPAQUE (type))
>  return type_die;
>else
>  remove_AT (type_die, DW_AT_declaration);
> @@ -21871,10 +21870,14 @@ gen_enumeration_type_die (tree type, dw_
>  {
>tree link;
>
> -  TREE_ASM_WRITTEN (type) = 1;
> -  add_byte_size_attribute (type_die, type);
> -  add_alignment_attribute (type_die, type);
> -  if (dwarf_version >= 3 || !dwarf_strict)
> +  if (!ENUM_IS_OPAQUE (type))
> +   TREE_ASM_WRITTEN (type) = 1;
> +  if (!orig_type_die || !get_AT (type_die, DW_AT_byte_size))
> +   add_byte_size_attribute (type_die, type);
> +  if (!orig_type_die || !get_AT (type_die, DW_AT_alignment))
> +   add_alignment_attribute (type_die, type);
> +  if ((dwarf_version >= 3 || !dwarf_strict)
> + && (!orig_type_die || !get_AT (type_die, DW_AT_type)))
> {
>   tree underlying = lang_hooks.types.enum_underlying_base_type (type);
>   add_type_attribute (type_die, underlying, TYPE_UNQUALIFIED, false,
> @@ -21882,8 +21885,10 @@ gen_enumeration_type_die (tree type, dw_
> }
>if (TYPE_STUB_DECL (type) != NULL_TREE)
> {
> - add_src_coords_attributes (type_die, TYPE_STUB_DECL (type));
> - 

[PATCH] Fix scoped enum debug info (PR debug/58150)

[Jakub Jelinek]

Hi!

As the following testcase shows, we emit bad debug info if a scoped
enum is used before the enumerators are defined.
gen_enumeration_type_die has support for enum forward declarations that
have NULL TYPE_SIZE (i.e. incomplete), but in this case it is complete,
just is ENUM_IS_OPAQUE and the enumerators aren't there.  We still set
TREE_ASM_WRITTEN on it and therefore don't do anything when it actually is
completed.

The following patch does change/fix a bunch of things:
1) I don't see the point of guarding the addition of DW_AT_declaration
to just -gdwarf-4 or -gno-strict-dwarf, that is already DWARF2 attribute and
we are emitting it for the forward declarations already
2) we don't set TREE_ASM_WRITTEN if ENUM_IS_OPAQUE
3) we don't try to do anything if it is ENUM_IS_OPAQUE that hasn't been
newly created; similarly to incomplete forward redeclarations, we should
have provided all that is needed on the first declaration
4) the addition of most attributes guarded with TYPE_SIZE is guarded by
(!original_type_die || !get_AT (type_die, DW_AT_...)); the
!original_type_die || part is just an optimization, so we don't try to query
it in the common cases where we are creating a fresh type die; anyway, the
reason for the guards is that we can now do the code twice for the same
type_die (on declaration and definition), and don't want to add the attributes
twice
5) not sure if ENUM_IS_OPAQUE must always imply non-NULL TYPE_SIZE, if not,
in that case we'd add the DW_AT_deprecated attribute twice

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-03-09  Jakub Jelinek  

PR debug/58150
* dwarf2out.c (gen_enumeration_type_die): Don't guard adding
DW_AT_declaration for ENUM_IS_OPAQUE on -gdwarf-4 or -gno-strict-dwarf,
but on TYPE_SIZE.  Don't do anything for ENUM_IS_OPAQUE if not creating
a new die.  Don't set TREE_ASM_WRITTEN if ENUM_IS_OPAQUE.  Guard
addition of most attributes on !orig_type_die or the attribute not
being present already.  Assert TYPE_VALUES is NULL for ENUM_IS_OPAQUE.

* g++.dg/debug/dwarf2/enum2.C: New test.

--- gcc/dwarf2out.c.jj  2018-03-08 22:49:41.965225512 +0100
+++ gcc/dwarf2out.c 2018-03-09 14:53:15.596115625 +0100
@@ -21839,6 +21839,7 @@ static dw_die_ref
 gen_enumeration_type_die (tree type, dw_die_ref context_die)
 {
   dw_die_ref type_die = lookup_type_die (type);
+  dw_die_ref orig_type_die = type_die;
 
   if (type_die == NULL)
 {
@@ -21846,20 +21847,18 @@ gen_enumeration_type_die (tree type, dw_
  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)
-   {
- if (ENUM_IS_SCOPED (type))
-   add_AT_flag (type_die, DW_AT_enum_class, 1);
- if (ENUM_IS_OPAQUE (type))
-   add_AT_flag (type_die, DW_AT_declaration, 1);
-   }
+  if ((dwarf_version >= 4 || !dwarf_strict)
+ && ENUM_IS_SCOPED (type))
+   add_AT_flag (type_die, DW_AT_enum_class, 1);
+  if (ENUM_IS_OPAQUE (type) && TYPE_SIZE (type))
+   add_AT_flag (type_die, DW_AT_declaration, 1);
   if (!dwarf_strict)
add_AT_unsigned (type_die, DW_AT_encoding,
 TYPE_UNSIGNED (type)
 ? DW_ATE_unsigned
 : DW_ATE_signed);
 }
-  else if (! TYPE_SIZE (type))
+  else if (! TYPE_SIZE (type) || ENUM_IS_OPAQUE (type))
 return type_die;
   else
 remove_AT (type_die, DW_AT_declaration);
@@ -21871,10 +21870,14 @@ gen_enumeration_type_die (tree type, dw_
 {
   tree link;
 
-  TREE_ASM_WRITTEN (type) = 1;
-  add_byte_size_attribute (type_die, type);
-  add_alignment_attribute (type_die, type);
-  if (dwarf_version >= 3 || !dwarf_strict)
+  if (!ENUM_IS_OPAQUE (type))
+   TREE_ASM_WRITTEN (type) = 1;
+  if (!orig_type_die || !get_AT (type_die, DW_AT_byte_size))
+   add_byte_size_attribute (type_die, type);
+  if (!orig_type_die || !get_AT (type_die, DW_AT_alignment))
+   add_alignment_attribute (type_die, type);
+  if ((dwarf_version >= 3 || !dwarf_strict)
+ && (!orig_type_die || !get_AT (type_die, DW_AT_type)))
{
  tree underlying = lang_hooks.types.enum_underlying_base_type (type);
  add_type_attribute (type_die, underlying, TYPE_UNQUALIFIED, false,
@@ -21882,8 +21885,10 @@ gen_enumeration_type_die (tree type, dw_
}
   if (TYPE_STUB_DECL (type) != NULL_TREE)
{
- add_src_coords_attributes (type_die, TYPE_STUB_DECL (type));
- add_accessibility_attribute (type_die, TYPE_STUB_DECL (type));
+ if (!orig_type_die || !get_AT (type_die, DW_AT_decl_file))
+   add_src_coords_attributes (type_die, TYPE_STUB_DECL (type));
+ if (!orig_type_die || !get_AT (type_die, DW_AT_accessibility))
+