On Thu, Aug 1, 2019, 6:01 PM Martin Sebor <mse...@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
