Re: [review] slotinfo in struct dtv_slotinfo_list should be flexible array [BZ #25...

[Florian Weimer <fweimer at redhat dot com>]

* Szabolcs Nagy:

> On 03/11/2019 17:11, Florian Weimer (Code Review) wrote:
>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489
>
> i can't login to gerrit. is there something special i need to do?

You need to create an account.  I can't seem to send a direct link to
the page, but there is a Register link on the sign in page.

Afterwards, someone needs to add you to the glibc maintainers group, so
that you can give +2s.

>> slotinfo in struct dtv_slotinfo_list should be flexible array [BZ #25097]
>> 
>> GCC 10 will warn about subscribing inner length zero arrays.  Use a GCC
>> extension in csu/libc-tls.c to allocate space for the static_slotinfo
>> variable.  Adjust nptl_db so that the type description machinery does
>> not attempt to determine the size of the flexible array member slotinfo.
>
> the patch looks ok to me.

Thanks.

>> -static struct
>> -{
>> -  struct dtv_slotinfo_list si;
>> -  /* The dtv_slotinfo_list data structure does not include the actual
>> -     information since it is defined as an array of size zero.  We define
>> -     here the necessary entries.  Note that it is not important whether
>> -     there is padding or not since we will always access the information
>> -     through the 'si' element.  */
>> -  struct dtv_slotinfo info[2 + TLS_SLOTINFO_SURPLUS];
>> -} static_slotinfo;
>> -
>> +static struct dtv_slotinfo_list static_slotinfo =
>> +  {
>> +   /* Allocate an array of 2 + TLS_SLOTINFO_SURPLUS elements.  */
>> +   .slotinfo =  { [array_length (_dl_static_dtv) - 1] = { } },
>> +  };
>
> i'd use {0} instead of {} as that's the universal initializer in c.

Is it?  I don't think that works if the array length is an aggregate.
It's true that { } is a GNU extension.

> (to me the original code looked more obvious)

Maybe, but it's undefined. 8-/

>>  static void
>>  init_slotinfo (void)
>>  {
>> -  /* Create the slotinfo list.  */
>> -  static_slotinfo.si.len = (((char *) (&static_slotinfo + 1)
>> -			     - (char *) &static_slotinfo.si.slotinfo[0])
>> -			    / sizeof static_slotinfo.si.slotinfo[0]);
>> -  // static_slotinfo.si.next = NULL;	already zero
>> +  /* Create the slotinfo list.  Note that the type of static_slotinfo
>> +     has effectively a zero-length array, so we cannot use the size of
>> +     static_slotinfo to determine the array length.  */
>> +  static_slotinfo.len = array_length (_dl_static_dtv);
>> +  /* static_slotinfo.si.next = NULL; -- Already zero.  */
>
> i'd remove the .si in the comment (or remove that comment)

Good point.  I have to resubmit the series anyway, and will fix it.

Thanks,
Florian