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

[Jason Merrill]

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.

> 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.

Jason