Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

On Sat, Sep 20 2014, Mark Wielaard wrote:

> When adding DW_TAG_restrict_type I made a mistake when updating the
> code that handled types with multiple modifiers. This patch fixes it
> by putting the logic for finding the "sub-qualified" type in a separate
> function and fall back to adding the modifiers separately if there is
> no such existing type. The old tests didn't catch this case because
> there always was an existing sub-qualified type already. The new testcase
> fails before and succeeds after this patch.
>
> gcc/ChangeLog
>
> 	* dwarf2out.c (existing_sub_qualified_type): New function.
> 	(modified_type_die): Use existing_sub_qualified_type. Fall
> 	back to adding modifiers one by one of there is no existing
> 	sub-qualified type.
>
> gcc/testsuite/ChangeLog
>
> 	* gcc.dg/guality/pr63300-const-volatile.c: New testcase.
> ---
>  gcc/dwarf2out.c                                    | 85 ++++++++++++++++++----
>  .../gcc.dg/guality/pr63300-const-volatile.c        | 12 +++
>  2 files changed, 84 insertions(+), 13 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index e87ade2..0cbc316 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -10461,6 +10461,51 @@ decl_quals (const_tree decl)
>  	     ? TYPE_QUAL_VOLATILE : TYPE_UNQUALIFIED));
>  }
>  
> +/* Returns true if CV_QUALS contains QUAL and we have a qualified
> +   variant of TYPE that has at least one other qualifier found in
> +   CV_QUALS.  Returns false if CV_QUALS doesn't contain QUAL, if
> +   CV_QUALS is empty after subtracting QUAL, or if we don't have a
> +   TYPE that has at least one qualifier from CV_QUALS minus QUAL.  */
> +static bool
> +existing_sub_qualified_type (tree type, int cv_quals, int qual)
> +{
> +  int sub_qual, sub_quals = cv_quals & ~qual;
> +  if ((cv_quals & qual) == TYPE_UNQUALIFIED || sub_quals == TYPE_UNQUALIFIED)
> +    return false;
> +
> +  sub_qual = TYPE_QUAL_CONST;
> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  sub_qual = TYPE_QUAL_VOLATILE;
> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  sub_qual = TYPE_QUAL_RESTRICT;
> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_VOLATILE;

You probably mean '|' instead of '&' here.

> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_RESTRICT;

See above.

> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  sub_qual = TYPE_QUAL_VOLATILE & TYPE_QUAL_RESTRICT;

See above.

> +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +    return true;
> +
> +  return false;
> +}

IIUC, 'sub_qual' above represents the qualifiers to *omit* from the ones
we're interested in, right?  Maybe it would be more straightforward to
reverse the logic, i.e., start with

        sub_qual = TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT;

and then always use sub_qual instead of ~sub_qual.

Also note that the logic wouldn't scale too well for yet more
qualifiers...

> +
>  /* Given a pointer to an arbitrary ..._TYPE tree node, return a debugging
>     entry that chains various modifiers in front of the given type.  */
>  
> @@ -10543,34 +10588,48 @@ modified_type_die (tree type, int cv_quals, dw_die_ref context_die)
>  
>    mod_scope = scope_die_for (type, context_die);
>  
> -  if ((cv_quals & TYPE_QUAL_CONST)
> -      /* If there are multiple type modifiers, prefer a path which
> -	 leads to a qualified type.  */
> -      && (((cv_quals & ~TYPE_QUAL_CONST) == TYPE_UNQUALIFIED)
> -	  || get_qualified_type (type, cv_quals) == NULL_TREE
> -	  || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_CONST)
> -	      != NULL_TREE)))
> +  /* If there are multiple type modifiers, prefer a path which
> +     leads to a qualified type.  */
> +  if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_CONST))
>      {
>        mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
>        sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
>  				   context_die);
>      }
> -  else if ((cv_quals & TYPE_QUAL_VOLATILE)
> -	   && (((cv_quals & ~TYPE_QUAL_VOLATILE) == TYPE_UNQUALIFIED)
> -	       || get_qualified_type (type, cv_quals) == NULL_TREE
> -	       || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_VOLATILE)
> -		   != NULL_TREE)))
> +  else if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_VOLATILE))
>      {
>        mod_type_die = new_die (DW_TAG_volatile_type, mod_scope, type);
>        sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_VOLATILE,
>  				   context_die);
>      }
> -  else if (cv_quals & TYPE_QUAL_RESTRICT)
> +  else if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_RESTRICT))
>      {
>        mod_type_die = new_die (DW_TAG_restrict_type, mod_scope, type);
>        sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_RESTRICT,
>  				   context_die);
>      }
> +  else if (cv_quals)
> +    {
> +      /* No existing path, just add qualifiers one by one.  */
> +      if (cv_quals & TYPE_QUAL_CONST)
> +	{
> +	  mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
> +	  sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
> +				       context_die);
> +	}
> +      else if (cv_quals & TYPE_QUAL_VOLATILE)
> +	{
> +	  mod_type_die = new_die (DW_TAG_volatile_type, mod_scope, type);
> +	  sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_VOLATILE,
> +				       context_die);
> +	}
> +      else
> +	{
> +	  mod_type_die = new_die (DW_TAG_restrict_type, mod_scope, type);
> +	  sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_RESTRICT,
> +				       context_die);
> +	}
> +    }
>    else if (code == POINTER_TYPE)
>      {
>        mod_type_die = new_die (DW_TAG_pointer_type, mod_scope, type);
> diff --git a/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c b/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
> new file mode 100644
> index 0000000..b8d75ed
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
> @@ -0,0 +1,12 @@
> +/* PR63300 'const volatile' sometimes stripped in debug info */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +int
> +main (int argc, char **argv)
> +{
> +  const volatile int v = argc;
> +  return v - argc;
> +}
> +
> +/* { dg-final { gdb-test 9 "type:v" "const volatile int" } } */