Re: [PATCH] fix #69251 - [6 Regression] ICE in unify_array_domain on a flexible array member

[Jason Merrill <jason at redhat dot com>]

On 02/02/2016 08:21 PM, Martin Sebor wrote:
> On 02/02/2016 05:28 AM, Jason Merrill wrote:
> > On 01/25/2016 05:55 PM, Martin Sebor wrote:
> > > The downside of this approach is that it prevents everything but
> > > the front end from distinguishing flexible array members from
> > > arrays of unspecified or unknown bounds.  The immediate impact
> > > is that prevents us from maintaining ABI compatibility with GCC
> > > 5 (with -fabi-version=9) and from diagnosing the mangling change.
> > > This means should we decide to adopt this approach, the final
> > > version of the patch for c++/69277 mentioned above that's still
> > > pending approval will need to be tweaked to have the ABI checks
> > > removed.
> >
> > That's unfortunate, but I think acceptable.
> >
> > >     * decl.c (compute_array_index_type): Return null for flexible array
> > >     members.
> >
> > Instead of this, I would think we can remove the calls to
> > compute_array_index_type added by your earlier patch, as well as many
> > other changes from that patch to handle null TYPE_MAX_VALUE.
>
> Yes, that's possible but it didn't seem essential at this stage.
> I wanted to make only conservative changes to avoid any further
> fallout.  I also wasn't sure whether the ABI issue above would
> make this approach unviable.

I guess my sense of conservatism is different from yours: I think 
removing recent changes is conservative in that it minimizes the change 
from previous versions of the compiler.

> > >     * tree.c (array_of_runtime_bound_p): Handle gracefully array types
> > >     with null TYPE_MAX_VALUE.
> >
> > This seems unneeded.
> >
> > >     (build_ctor_subob_ref): Loosen debug checking to handle flexible
> > >     array members.
> >
> > And this shouldn't need the TYPE_MAX_VALUE check.
>
> I went ahead and made the requested changes.  They might seem
> perfectly innocuous to you but the removal of the tests for
> TYPE_MAX_VALUE(t) being null makes me nervous at this stage.
> I'm not nearly comfortable enough with the code to be confident
> that they're all 100% safe.  I defer to your better judgment
> on this.

It was impossible to have null TYPE_MAX_VALUE until you introduced that 
in compute_array_index_type, and thus we didn't test for it; if we 
aren't doing that anymore I can't imagine where it would come from now.

> @@ -4120,9 +4120,8 @@ walk_subobject_offsets (tree type,
> -	  || !domain
> -	  /* Flexible array members have no upper bound.  */
> -	  || !TYPE_MAX_VALUE (domain))
> +	  /* Flexible array members have a null domain.  */
> +	  || !domain)

With this patch flexible array members are a special case of array of 
unknown bound, so I don't think we need to call them out in a comment here.

> @@ -875,10 +875,11 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags)
> -      if (TYPE_DOMAIN (t) && TYPE_MAX_VALUE (TYPE_DOMAIN (t)))
> +      /* C++ flexible array members have a null domain.  */
> +      if (tree dtype = TYPE_DOMAIN (t))

Likewise.

OK with these two comments removed.

Jason