Fix middle-end/93961

Message ID 2013248.zzpuD0pr01@polaris
State New
Headers show
Series
  • Fix middle-end/93961
Related show

Commit Message

Eric Botcazou March 11, 2020, 7:49 a.m.
Hi,

this is a GIMPLE verification failure in LTO mode on Ada code:

/vol/gcc/src/hg/master/local/gcc/testsuite/gnat.dg/lto24_pkg1.ads:9:8: error: 
type mismatch in 'component_ref'
lto24_pkg1__rec___b___XVN

lto24_pkg1__rec___b___XVN

The __XVN fields are the fields with QUAL_UNION_TYPE in Ada, which is the only 
language using this type.  The issue is that tree_is_indexable doesn't return 
the same result for a FIELD_DECL with QUAL_UNION_TYPE and the QUAL_UNION_TYPE, 
resulting in two instances of the QUAL_UNION_TYPE in the bytecode.  The result
for the type is the correct one (false, since it is variably modified) while 
the result for the field is falsely true because:

  else if (TREE_CODE (t) == FIELD_DECL
	   && lto_variably_modified_type_p (DECL_CONTEXT (t)))
    return false;

is not satisfied.  The reason for this is that the DECL_QUALIFIER of fields of 
a QUAL_UNION_TYPE depends on a discriminant in Ada, which means that the size 
of the type does too (CONTAINS_PLACEHOLDER_P), which in turn means that it is 
reset to a mere PLACEHOLDER_EXPR by free_lang_data, which finally means that 
the size of DECL_CONTEXT is too, so RETURN_TRUE_IF_VAR is false.

In other words, the CONTAINS_PLACEHOLDER_P property of the DECL_QUALIFIER of 
fields of a QUAL_UNION_TYPE hides the variably_modified_type_p property of 
these fields, if you look from the outside.

This was clearly overlooked (by me) when the handling of PLACEHOLDER_EXPR was 
added to LTO a decade ago, but I think that it's now too late to fundamentally 
change how this is done, so I propose the attached fix.

Tested on SPARC/Solaris and x86-64/Linux, OK for the mainline?  It's not a 
regression, but the fix is a no-op except for Ada and we have been using it 
internally for some time without any issue so far.


2020-03-11  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/93961
	* tree.c (variably_modified_type_p) <RECORD_TYPE>: Recurse into fields
	whose type is a qualified union.

-- 
Eric Botcazou

Comments

will schmidt via Gcc-patches March 11, 2020, 8:30 a.m. | #1
On Wed, Mar 11, 2020 at 8:56 AM Eric Botcazou <ebotcazou@adacore.com> wrote:
>

> Hi,

>

> this is a GIMPLE verification failure in LTO mode on Ada code:

>

> /vol/gcc/src/hg/master/local/gcc/testsuite/gnat.dg/lto24_pkg1.ads:9:8: error:

> type mismatch in 'component_ref'

> lto24_pkg1__rec___b___XVN

>

> lto24_pkg1__rec___b___XVN

>

> The __XVN fields are the fields with QUAL_UNION_TYPE in Ada, which is the only

> language using this type.  The issue is that tree_is_indexable doesn't return

> the same result for a FIELD_DECL with QUAL_UNION_TYPE and the QUAL_UNION_TYPE,

> resulting in two instances of the QUAL_UNION_TYPE in the bytecode.  The result

> for the type is the correct one (false, since it is variably modified) while

> the result for the field is falsely true because:

>

>   else if (TREE_CODE (t) == FIELD_DECL

>            && lto_variably_modified_type_p (DECL_CONTEXT (t)))

>     return false;

>

> is not satisfied.  The reason for this is that the DECL_QUALIFIER of fields of

> a QUAL_UNION_TYPE depends on a discriminant in Ada, which means that the size

> of the type does too (CONTAINS_PLACEHOLDER_P), which in turn means that it is

> reset to a mere PLACEHOLDER_EXPR by free_lang_data, which finally means that

> the size of DECL_CONTEXT is too, so RETURN_TRUE_IF_VAR is false.

>

> In other words, the CONTAINS_PLACEHOLDER_P property of the DECL_QUALIFIER of

> fields of a QUAL_UNION_TYPE hides the variably_modified_type_p property of

> these fields, if you look from the outside.

>

> This was clearly overlooked (by me) when the handling of PLACEHOLDER_EXPR was

> added to LTO a decade ago, but I think that it's now too late to fundamentally

> change how this is done, so I propose the attached fix.

>

> Tested on SPARC/Solaris and x86-64/Linux, OK for the mainline?  It's not a

> regression, but the fix is a no-op except for Ada and we have been using it

> internally for some time without any issue so far.


OK.  Note I wondered for some time whether we can pre-compute (and LTO stream)
whether a type is variably modified during type layout?

Thanks,
Richard.

>

> 2020-03-11  Eric Botcazou  <ebotcazou@adacore.com>

>

>         PR middle-end/93961

>         * tree.c (variably_modified_type_p) <RECORD_TYPE>: Recurse into fields

>         whose type is a qualified union.

>

> --

> Eric Botcazou
Eric Botcazou March 11, 2020, 10:26 a.m. | #2
> OK.  Note I wondered for some time whether we can pre-compute (and LTO

> stream) whether a type is variably modified during type layout?


During type layout seems a little early, the end of gimplification would seem 
more natural since we have a workaround in RETURN_TRUE_IF_VAR:

/* Test if T is either variable (if FN is zero) or an expression containing
   a variable in FN.  If TYPE isn't gimplified, return true also if
   gimplify_one_sizepos would gimplify the expression into a local
   variable.  */

-- 
Eric Botcazou

Patch

diff --git a/gcc/tree.c b/gcc/tree.c
index 66d52c71c99..905563fa4be 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9206,8 +9206,18 @@  variably_modified_type_p (tree type, tree fn)
 	    RETURN_TRUE_IF_VAR (DECL_SIZE (t));
 	    RETURN_TRUE_IF_VAR (DECL_SIZE_UNIT (t));
 
+	    /* If the type is a qualified union, then the DECL_QUALIFIER
+	       of fields can also be an expression containing a variable.  */
 	    if (TREE_CODE (type) == QUAL_UNION_TYPE)
 	      RETURN_TRUE_IF_VAR (DECL_QUALIFIER (t));
+
+	    /* If the field is a qualified union, then it's only a container
+	       for what's inside so we look into it.  That's necessary in LTO
+	       mode because the sizes of the field tested above have been set
+	       to PLACEHOLDER_EXPRs by free_lang_data.  */
+	    if (TREE_CODE (TREE_TYPE (t)) == QUAL_UNION_TYPE
+		&& variably_modified_type_p (TREE_TYPE (t), fn))
+	      return true;
 	  }
       break;