Re: [PATCH] don't trim empty string initializers for pointers (PR 90947)

[Jason Merrill <jason at redhat dot com>]

On Thu, Aug 1, 2019, 6:01 PM Martin Sebor <msebor@gmail.com> wrote:

> On 8/1/19 3:09 PM, Jason Merrill wrote:
> > On 8/1/19 4:22 PM, Martin Sebor wrote:
> >> On 6/27/19 3:25 PM, Jason Merrill wrote:
> >>> On 6/23/19 5:51 PM, Martin Sebor wrote:
> >>>> On 6/22/19 9:37 PM, Jason Merrill wrote:
> >>>>> On 6/21/19 8:05 PM, Martin Sebor wrote:
> >>>>>> The solution we implemented in GCC 9 to get the mangling of
> >>>>>> non-type template arguments of class types containing array
> >>>>>> members consistent regardless of the form of their
> >>>>>> initialization introduced a couple of bugs.  One of these
> >>>>>> is the subject of this patch.  The bug results in stripping
> >>>>>> trailing initializers for array elements that involve the empty
> >>>>>> string such as in here:
> >>>>>>
> >>>>>>    void f (void)
> >>>>>>    {
> >>>>>>      const char* a[1][1] = { "" };
> >>>>>>      if (!a[0][0])
> >>>>>>        __builtin_abort ();
> >>>>>>    }
> >>>>>>
> >>>>>> The problem is caused by relying on initializer_zerop() that
> >>>>>> returns true for the empty string regardless of whether it's
> >>>>>> used to initialize an array or a pointer (the function doesn't
> >>>>>> know what the initializer is being used for).
> >>>>>
> >>>>> Why doesn't the existing POINTER_TYPE_P check handle this?  It's
> >>>>> clearly intended to.
> >>>>
> >>>> It does but only only for initializers of "leaf" elements/members.
> >>>> The function processes initializers of enclosing elements or members
> >>>> recursively.  When initializer_zerop returns true for one of those,
> >>>> it doesn't differentiate between an empty string that's being used
> >>>> to initialize a leaf array or a leaf pointer.
> >>>>
> >>>> For the example above, the first time through, elt_type is char*
> >>>> and elt_init is the empty string, or the array char[1], and so
> >>>> the initializer is retained.
> >>>>
> >>>> The second time through (after the leaf recursive call returns),
> >>>> elt_init is { "" } (i.e., an array of one empty string), elt_type
> >>>> is also an array, and initializer_zerop(elt_init) returns true, so
> >>>> the initializer is dropped.
> >>>>
> >>>> I'm actually surprised this initializer_zerop ambiguity between
> >>>> arrays and strings doesn't come up elsewhere.  That's also why
> >>>> I added the new function to tree.c.
> >>>
> >>> Makes sense.
> >>>
> >>>> Btw., it belatedly occurred to me that the new function doesn't
> >>>> correctly handled unnamed bit-fields so I've corrected it to
> >>>> handle those as well.  Attached is the revised patch with this
> >>>> change and a test to exercise it.
> >>>
> >>>> +         || (DECL_BIT_FIELD (fld) && !!DECL_NAME (fld)))
> >>>
> >>> Hmm, this looks like it would skip named bit-fields rather than
> unnamed.
> >>
> >> It was a typo that I fixed by hand separately in my tree and in
> >> the patch but missed Emacs' question when saving the patch and
> >> wound up sending the unchanged version.
> >>
> >>>> PS I found DECL_UNNAMED_BIT_FIELD in c-family/c-common.h.
> >>>> I used (DECL_BIT_FIELD (fld) && !DECL_NAME (fld)) because
> >>>> the former macro isn't available in tree.c.  Does that mean
> >>>> that that would make the new function not completely correct
> >>>> in some other languages?
> >>>
> >>> I don't know if other languages treat unnamed bit-fields as
> >>> initializable.
> >>>
> >>>> +/* Analogous to initializer_zerop but examines the type for which
> >>>> +   the initializer is being used but unlike it, considers empty
> >>>> +   strings to be zero initializers for arrays and non-zero for
> >>>> +   pointers.  */
> >>>
> >>> This comment could use more editing.
> >>
> >> I've broken up the long sentence into two so it reads a little
> >> better.  If you were objecting to something else please let me
> >> know.
> >>
> >> Other than that and the typo the patch is the same.
> >
> > OK, thanks.
>
> Just to be sure before I commit it: is this OK meant as an approval
> of the patch (as opposed to just an acknowledgment of the changes)?
>

Yes.

Jason