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

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489
......................................................................

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.

Change-Id: I51be146a7857186a4ede0bb40b332509487bdde8
---
M csu/libc-tls.c
M nptl_db/db-symbols.h
M nptl_db/db_info.c
M nptl_db/structs.def
M nptl_db/thread_dbP.h
M sysdeps/generic/ldsodefs.h
6 files changed, 29 insertions(+), 22 deletions(-)

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?

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

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

(to me the original code looked more obvious)

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

* 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

On 06/11/2019 16:21, Florian Weimer wrote:
> * 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.

that's what i expected but it does not work for me.

and if i try to register again it says email already exist,
but i've never got a verification mail (i tried the resend
link several times).

creating another account with different email didn't work either
(first i missed the timeout: i got the mail 10 minutes after
registration and it said the verification link expires after
15 min.. then after a few tries i managed to follow the verification
link but it just says 'Forbidden' and all later site access just
kept saying forbidden even though i never asked the site to
remember my login.. i had to clear all the site related cookies
to be able to visit the registration site again)

i'm a bit lost how to debug this, i guess i'll stay away from
gerrit until there is a solution that works.

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

* Szabolcs Nagy:

> On 06/11/2019 16:21, Florian Weimer wrote:
>> * 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.
>
> that's what i expected but it does not work for me.
>
> and if i try to register again it says email already exist,
> but i've never got a verification mail (i tried the resend
> link several times).
>
> creating another account with different email didn't work either
> (first i missed the timeout: i got the mail 10 minutes after
> registration and it said the verification link expires after
> 15 min.. then after a few tries i managed to follow the verification
> link but it just says 'Forbidden' and all later site access just
> kept saying forbidden even though i never asked the site to
> remember my login.. i had to clear all the site related cookies
> to be able to visit the registration site again)
>
> i'm a bit lost how to debug this, i guess i'll stay away from
> gerrit until there is a solution that works.

Sorry, I don't have access to this system.

Simon, Carlos, would you please help Szabolcs with this?

Thanks,
Florian

On Monday, November 11 2019, Florian Weimer wrote:

> * Szabolcs Nagy:
>
>> On 06/11/2019 16:21, Florian Weimer wrote:
>>> * 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.
>>
>> that's what i expected but it does not work for me.
>>
>> and if i try to register again it says email already exist,
>> but i've never got a verification mail (i tried the resend
>> link several times).
>>
>> creating another account with different email didn't work either
>> (first i missed the timeout: i got the mail 10 minutes after
>> registration and it said the verification link expires after
>> 15 min.. then after a few tries i managed to follow the verification
>> link but it just says 'Forbidden' and all later site access just
>> kept saying forbidden even though i never asked the site to
>> remember my login.. i had to clear all the site related cookies
>> to be able to visit the registration site again)
>>
>> i'm a bit lost how to debug this, i guess i'll stay away from
>> gerrit until there is a solution that works.
>
> Sorry, I don't have access to this system.
>
> Simon, Carlos, would you please help Szabolcs with this?

I've manually activated your account, Szabolcs.  I don't know why you
didn't receive the email; you should have.  I checked the postfix log on
the machine and saw that two emails have been sent to your address.
Maybe they were flagged as spam?  We have other people using @arm.com
emails registered, and they didn't have problems.

Thanks,

On 11/11/2019 21:41, Sergio Durigan Junior wrote:
> I've manually activated your account, Szabolcs.  I don't know why you
> didn't receive the email; you should have.  I checked the postfix log on
> the machine and saw that two emails have been sent to your address.
> Maybe they were flagged as spam?  We have other people using @arm.com
> emails registered, and they didn't have problems.

thanks, now login worked.

i clicked to resend the email more than 5 times
over a period of a week, so if there were only 2
emails sent that sounds wrong. i think arm might
have had temporary IT problems, but not for a
whole week.

* Szabolcs Nagy:

> On 11/11/2019 21:41, Sergio Durigan Junior wrote:
>> I've manually activated your account, Szabolcs.  I don't know why you
>> didn't receive the email; you should have.  I checked the postfix log on
>> the machine and saw that two emails have been sent to your address.
>> Maybe they were flagged as spam?  We have other people using @arm.com
>> emails registered, and they didn't have problems.
>
> thanks, now login worked.
>
> i clicked to resend the email more than 5 times
> over a period of a week, so if there were only 2
> emails sent that sounds wrong. i think arm might
> have had temporary IT problems, but not for a
> whole week.

It could be that the mail setup for osci.io is too non-standard for your
corporate mail provider.  Red Hat has outsourced its inbound mail relays
over the weekend, so I can sympathize. 8-/

Sergio, have you been in contact with the people at osci.io?  I can talk
to them about fixing some of the issues I see, but whether that removes
the remaining obstacles to successful mail delivery is hard to predict.

Thanks,
Florian

Szabolcs Nagy has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

two nits, otherwise ok.

| --- csu/libc-tls.c
| +++ csu/libc-tls.c
| @@ -199,18 +193,18 @@ #endif
|    /* Update the executable's link map with enough information to make
|       the TLS routines happy.  */
|    main_map->l_tls_align = align;
|    main_map->l_tls_blocksize = memsz;
|    main_map->l_tls_initimage = initimage;
|    main_map->l_tls_initimage_size = filesz;
|    main_map->l_tls_modid = 1;
|  
|    init_slotinfo ();
|    // static_slotinfo.si.slotinfo[1].gen = 0; already zero

PS2, Line 202:

.si in the comment is wrong

| -  static_slotinfo.si.slotinfo[1].map = main_map;
| +  static_slotinfo.slotinfo[1].map = main_map;
|  
|    memsz = roundup (memsz, align ?: 1);
|  
|  #if TLS_DTV_AT_TP
|    memsz += tcb_offset;
|  #endif
|  
| --- nptl_db/db_info.c
| +++ nptl_db/db_info.c
| @@ -49,15 +49,18 @@ #define schedparam_sched_priority schedparam.sched_priority
|  
|  #define eventbuf_eventmask eventbuf.eventmask
|  #define eventbuf_eventmask_event_bits eventbuf.eventmask.event_bits
|  
|  #define DESC(name, offset, obj) \
|    DB_DEFINE_DESC (name, 8 * sizeof (obj), 1, offset);
|  #define ARRAY_DESC(name, offset, obj) \
|    DB_DEFINE_DESC (name, \
| -		  8 * sizeof (obj)[0], sizeof (obj) / sizeof (obj)[0], \
| +		  8 * sizeof (obj)[0], sizeof (obj) / sizeof (obj)[0],	\

PS2, Line 57:

is this whitespace change useful?

|  		  offset);
| +/* Flexible arrays do not have a length that can be determined.  */
| +#define FLEXIBLE_ARRAY_DESC(name, offset, obj) \
| +  DB_DEFINE_DESC (name, 8 * sizeof (obj)[0], 0, offset);
|  
|  #if TLS_TCB_AT_TP
|  # define dtvp header.dtv
|  #elif TLS_DTV_AT_TP
|  /* Special case hack.  If TLS_TCB_SIZE == 0 (on PowerPC), there is no TCB

Florian Weimer has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489
......................................................................


Patch Set 3:

(2 comments)

This should be ready for committing.

| --- csu/libc-tls.c
| +++ csu/libc-tls.c
| @@ -199,18 +193,18 @@ #endif
|    /* Update the executable's link map with enough information to make
|       the TLS routines happy.  */
|    main_map->l_tls_align = align;
|    main_map->l_tls_blocksize = memsz;
|    main_map->l_tls_initimage = initimage;
|    main_map->l_tls_initimage_size = filesz;
|    main_map->l_tls_modid = 1;
|  
|    init_slotinfo ();
|    // static_slotinfo.si.slotinfo[1].gen = 0; already zero

PS2, Line 202:

Right, fixed. That removes the last .si reference from the file.

| -  static_slotinfo.si.slotinfo[1].map = main_map;
| +  static_slotinfo.slotinfo[1].map = main_map;
|  
|    memsz = roundup (memsz, align ?: 1);
|  
|  #if TLS_DTV_AT_TP
|    memsz += tcb_offset;
|  #endif
|  
| --- nptl_db/db_info.c
| +++ nptl_db/db_info.c
| @@ -49,15 +49,18 @@ #define schedparam_sched_priority schedparam.sched_priority
|  
|  #define eventbuf_eventmask eventbuf.eventmask
|  #define eventbuf_eventmask_event_bits eventbuf.eventmask.event_bits
|  
|  #define DESC(name, offset, obj) \
|    DB_DEFINE_DESC (name, 8 * sizeof (obj), 1, offset);
|  #define ARRAY_DESC(name, offset, obj) \
|    DB_DEFINE_DESC (name, \
| -		  8 * sizeof (obj)[0], sizeof (obj) / sizeof (obj)[0], \
| +		  8 * sizeof (obj)[0], sizeof (obj) / sizeof (obj)[0],	\

PS2, Line 57:

No, it's not, it's a leftover from previous changes. Fixed.

|  		  offset);
| +/* Flexible arrays do not have a length that can be determined.  */
| +#define FLEXIBLE_ARRAY_DESC(name, offset, obj) \
| +  DB_DEFINE_DESC (name, 8 * sizeof (obj)[0], 0, offset);
|  
|  #if TLS_TCB_AT_TP
|  # define dtvp header.dtv
|  #elif TLS_DTV_AT_TP
|  /* Special case hack.  If TLS_TCB_SIZE == 0 (on PowerPC), there is no TCB

Szabolcs Nagy has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489
......................................................................


Patch Set 3: Code-Review+2

This change (glibc commit cba932a5a9e91cffd7f4172d7e91f9b2efb1f84b) has 
broken the glibc testsuite build for nios2.  I don't know whether the 
cause is some underlying bug in GCC or binutils or elsewhere in glibc, or 
somehow an issue with this patch itself.

/scratch/jmyers/glibc/many9/install/compilers/nios2-linux-gnu/lib/gcc/nios2-glibc-linux-gnu/9.2.1/../../../../nios2-glibc-linux-gnu/bin/ld: /scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/libc.a(libc-tls.o): in function `init_slotinfo':
/scratch/jmyers/glibc/many9/src/glibc/csu/../csu/libc-tls.c:72: warning: unable to reach .bss (at 0xc5ea0) from the global pointer (at 0x8d890) because the offset (230928) is out of the allowed range, -32678 to 32767

/scratch/jmyers/glibc/many9/install/compilers/nios2-linux-gnu/lib/gcc/nios2-glibc-linux-gnu/9.2.1/../../../../nios2-glibc-linux-gnu/bin/ld: final link failed: nonrepresentable section on output
collect2: error: ld returned 1 exit status
../Rules:253: recipe for target '/scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/malloc/tst-interpose-static-nothread' failed
make[3]: *** [/scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/malloc/tst-interpose-static-nothread] Error 1

https://sourceware.org/ml/libc-testresults/2019-q4/msg00203.html

On 11/12/19 10:29 AM, Joseph Myers wrote:
> This change (glibc commit cba932a5a9e91cffd7f4172d7e91f9b2efb1f84b) has
> broken the glibc testsuite build for nios2.  I don't know whether the
> cause is some underlying bug in GCC or binutils or elsewhere in glibc, or
> somehow an issue with this patch itself.
> 
> /scratch/jmyers/glibc/many9/install/compilers/nios2-linux-gnu/lib/gcc/nios2-glibc-linux-gnu/9.2.1/../../../../nios2-glibc-linux-gnu/bin/ld: /scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/libc.a(libc-tls.o): in function `init_slotinfo':
> /scratch/jmyers/glibc/many9/src/glibc/csu/../csu/libc-tls.c:72: warning: unable to reach .bss (at 0xc5ea0) from the global pointer (at 0x8d890) because the offset (230928) is out of the allowed range, -32678 to 32767
> 
> /scratch/jmyers/glibc/many9/install/compilers/nios2-linux-gnu/lib/gcc/nios2-glibc-linux-gnu/9.2.1/../../../../nios2-glibc-linux-gnu/bin/ld: final link failed: nonrepresentable section on output
> collect2: error: ld returned 1 exit status
> ../Rules:253: recipe for target '/scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/malloc/tst-interpose-static-nothread' failed
> make[3]: *** [/scratch/jmyers/glibc/many9/build/glibcs/nios2-linux-gnu/glibc/malloc/tst-interpose-static-nothread] Error 1
> 
> https://sourceware.org/ml/libc-testresults/2019-q4/msg00203.html
> 

I don't have any state on this particular change or what it is trying to 
accomplish, but the linker error is the sort of thing that happens when 
the compiler sees a reference to an object it thinks will be put in the 
small data section, but the actual definition of the object puts it 
somewhere else (e.g., because it is too big for small data) -- in this 
case the .bss section.  Are there conflicting declarations about the 
size of the object?  If that's unavoidable for some reason, another way 
to suppress GP-relative addressing on this object would be to give it an 
explicit .bss section attribute everywhere it's declared.

-Sandra

On Tue, 12 Nov 2019, Sandra Loosemore wrote:

> I don't have any state on this particular change or what it is trying to
> accomplish, but the linker error is the sort of thing that happens when the

It's one of the changes to fix the glibc build for new array bounds 
warnings in GCC 10 
<https://sourceware.org/bugzilla/show_bug.cgi?id=25097>.  (There are other 
such changes needed that haven't yet been reviewed / committed, so the 
build with GCC 10 is still broken everywhere; this nios2-specific issue 
showed up with my bots building with GCC 8 and 9, where the glibc build 
succeeds but then the testsuite build fails for nios2.  I don't know more 
about the details of this change.)

* Sandra Loosemore:

> I don't have any state on this particular change or what it is trying to 
> accomplish, but the linker error is the sort of thing that happens when 
> the compiler sees a reference to an object it thinks will be put in the 
> small data section, but the actual definition of the object puts it 
> somewhere else (e.g., because it is too big for small data) -- in this 
> case the .bss section.  Are there conflicting declarations about the 
> size of the object?  If that's unavoidable for some reason, another way 
> to suppress GP-relative addressing on this object would be to give it an 
> explicit .bss section attribute everywhere it's declared.

Thanks for providing this information.  Here is a small reproducer:

enum { size = 100 };

struct flexible
{
  int length;
  int data[];
};

struct inflexible
{
  int length;
  int data[size];
};

static struct flexible flexible =
  {
   .data = { [size - 1] = 0, }
  };

static struct inflexible inflexible =
  {
   .data = { [size - 1] = 0, }
  };

struct flexible *
get_flexible (void)
{
  return &flexible;
}

struct inflexible *
get_inflexible (void)
{
  return &inflexible;
}

It results in:

	.file	"t.c"
	.section	.text
	.align	2
	.global	get_flexible
	.type	get_flexible, @function
get_flexible:
	addi	r2, gp, %gprel(flexible)
	ret
	.size	get_flexible, .-get_flexible
	.align	2
	.global	get_inflexible
	.type	get_inflexible, @function
get_inflexible:
	movhi	r2, %hiadj(inflexible)
	addi	r2, r2, %lo(inflexible)
	ret
	.size	get_inflexible, .-get_inflexible
	.section	.bss
	.type	inflexible, @object
	.size	inflexible, 404
	.align	2
inflexible:
	.zero	404
	.type	flexible, @object
	.size	flexible, 404
	.align	2
flexible:
	.zero	404
	.ident	"GCC: (GNU) 9.2.1 20191101 [gcc-9-branch revision 277712]"

I think this shows that the backend uses the static type size (as in
sizeof) to determine whether an object goes into the small data
section, not the allocated object size.

The linker only sees the allocated object size, so it places the
object wrongly (although it could perhaps do something smarter because
it can see the relocations).

If the object is not placed into .bss, the choice of .sdata vs .data
is also based on the static type size, so that would need fixing too.

I don't know how to work around that in the source code.  My
preference would be to fix the backend.  I guess we could back out the
commit and disable the warning for GCC 10 instead.

On 11/12/19 11:42 AM, Florian Weimer wrote:
> * Sandra Loosemore:
> 
>> I don't have any state on this particular change or what it is trying to
>> accomplish, but the linker error is the sort of thing that happens when
>> the compiler sees a reference to an object it thinks will be put in the
>> small data section, but the actual definition of the object puts it
>> somewhere else (e.g., because it is too big for small data) -- in this
>> case the .bss section.  Are there conflicting declarations about the
>> size of the object?  If that's unavoidable for some reason, another way
>> to suppress GP-relative addressing on this object would be to give it an
>> explicit .bss section attribute everywhere it's declared.
> 
> Thanks for providing this information.  Here is a small reproducer:
> 
> enum { size = 100 };
> 
> struct flexible
> {
>    int length;
>    int data[];
> };
> 
> struct inflexible
> {
>    int length;
>    int data[size];
> };
> 
> static struct flexible flexible =
>    {
>     .data = { [size - 1] = 0, }
>    };
> 
> static struct inflexible inflexible =
>    {
>     .data = { [size - 1] = 0, }
>    };
> 
> struct flexible *
> get_flexible (void)
> {
>    return &flexible;
> }
> 
> struct inflexible *
> get_inflexible (void)
> {
>    return &inflexible;
> }
> 
> It results in:
> 
> 	.file	"t.c"
> 	.section	.text
> 	.align	2
> 	.global	get_flexible
> 	.type	get_flexible, @function
> get_flexible:
> 	addi	r2, gp, %gprel(flexible)
> 	ret
> 	.size	get_flexible, .-get_flexible
> 	.align	2
> 	.global	get_inflexible
> 	.type	get_inflexible, @function
> get_inflexible:
> 	movhi	r2, %hiadj(inflexible)
> 	addi	r2, r2, %lo(inflexible)
> 	ret
> 	.size	get_inflexible, .-get_inflexible
> 	.section	.bss
> 	.type	inflexible, @object
> 	.size	inflexible, 404
> 	.align	2
> inflexible:
> 	.zero	404
> 	.type	flexible, @object
> 	.size	flexible, 404
> 	.align	2
> flexible:
> 	.zero	404
> 	.ident	"GCC: (GNU) 9.2.1 20191101 [gcc-9-branch revision 277712]"
> 
> I think this shows that the backend uses the static type size (as in
> sizeof) to determine whether an object goes into the small data
> section, not the allocated object size.
> 
> The linker only sees the allocated object size, so it places the
> object wrongly (although it could perhaps do something smarter because
> it can see the relocations).
> 
> If the object is not placed into .bss, the choice of .sdata vs .data
> is also based on the static type size, so that would need fixing too.
> 
> I don't know how to work around that in the source code.  My
> preference would be to fix the backend.  I guess we could back out the
> commit and disable the warning for GCC 10 instead.
> 

OK, having a self-contained test case is very helpful.  I'll take a look 
at fixing this in GCC, although I might not get to it for a few days. 
Probably the solution is to not use GP-relative addressing for any 
object declared with a flexible array member.

-Sandra

On 11/12/19 1:42 PM, Florian Weimer wrote:
> * Sandra Loosemore:
> 
>> I don't have any state on this particular change or what it is trying to 
>> accomplish, but the linker error is the sort of thing that happens when 
>> the compiler sees a reference to an object it thinks will be put in the 
>> small data section, but the actual definition of the object puts it 
>> somewhere else (e.g., because it is too big for small data) -- in this 
>> case the .bss section.  Are there conflicting declarations about the 
>> size of the object?  If that's unavoidable for some reason, another way 
>> to suppress GP-relative addressing on this object would be to give it an 
>> explicit .bss section attribute everywhere it's declared.
> 
> Thanks for providing this information.  Here is a small reproducer:
> 
> enum { size = 100 };
> 
> struct flexible
> {
>   int length;
>   int data[];
> };
> 
> struct inflexible
> {
>   int length;
>   int data[size];
> };
> 
> static struct flexible flexible =
>   {
>    .data = { [size - 1] = 0, }
>   };
> 
> static struct inflexible inflexible =
>   {
>    .data = { [size - 1] = 0, }
>   };
> 
> struct flexible *
> get_flexible (void)
> {
>   return &flexible;
> }
> 
> struct inflexible *
> get_inflexible (void)
> {
>   return &inflexible;
> }
> 
> It results in:
> 
> 	.file	"t.c"
> 	.section	.text
> 	.align	2
> 	.global	get_flexible
> 	.type	get_flexible, @function
> get_flexible:
> 	addi	r2, gp, %gprel(flexible)
> 	ret
> 	.size	get_flexible, .-get_flexible
> 	.align	2
> 	.global	get_inflexible
> 	.type	get_inflexible, @function
> get_inflexible:
> 	movhi	r2, %hiadj(inflexible)
> 	addi	r2, r2, %lo(inflexible)
> 	ret
> 	.size	get_inflexible, .-get_inflexible
> 	.section	.bss
> 	.type	inflexible, @object
> 	.size	inflexible, 404
> 	.align	2
> inflexible:
> 	.zero	404
> 	.type	flexible, @object
> 	.size	flexible, 404
> 	.align	2
> flexible:
> 	.zero	404
> 	.ident	"GCC: (GNU) 9.2.1 20191101 [gcc-9-branch revision 277712]"
> 
> I think this shows that the backend uses the static type size (as in
> sizeof) to determine whether an object goes into the small data
> section, not the allocated object size.
> 
> The linker only sees the allocated object size, so it places the
> object wrongly (although it could perhaps do something smarter because
> it can see the relocations).
> 
> If the object is not placed into .bss, the choice of .sdata vs .data
> is also based on the static type size, so that would need fixing too.
> 
> I don't know how to work around that in the source code.  My
> preference would be to fix the backend.  I guess we could back out the
> commit and disable the warning for GCC 10 instead.
 
If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
a problem though. It immediately makes it difficult to test a new glibc on
existing systems without upgrading the system compiler.

If there is a backend bug then it absolutely needs to be fixed, and it looks
like Sandra will try that, but that won't reach the affected compilers until
much much later.

I dislike backing out the changes you made because they are quite nice
cleanups, and the downstream vendors tend to upgrade glibc and gcc and binutils
all in one big group to avoid these issues so I don't expect they will ever
see this problem. It's only us as developers that see the larger version
skew problem.

My inclination is to fix the gcc problem, backport the solution to the active
FSF branches, and then that fixes our testers.

In order of importance:
- Fix all gcc10 issues.
- Fix the nios2 backend issue.
- Backport nios2 backend to active FSF branches.
- Rebuild and test to make sure we are all clean again.

How long will that take and is it OK to leave things in a broken state for that long
for a given target like nios2?

* Carlos O'Donell:

> If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
> a problem though. It immediately makes it difficult to test a new glibc on
> existing systems without upgrading the system compiler.

I don't think nios2 systems have system compilers as such …

> How long will that take and is it OK to leave things in a broken
> state for that long for a given target like nios2?

Maybe we should back the fix out and see how quickly we can get in the
other GCC 10 patches?

On 11/12/19 5:18 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
>> a problem though. It immediately makes it difficult to test a new glibc on
>> existing systems without upgrading the system compiler.
> 
> I don't think nios2 systems have system compilers as such …

I don't really know what to call the SDK compiler that comes from Altera.

I'm going to call them "system compilers", though I'm open to other terms.

>> How long will that take and is it OK to leave things in a broken
>> state for that long for a given target like nios2?
> 
> Maybe we should back the fix out and see how quickly we can get in the
> other GCC 10 patches?

I'd like to hear Joseph's opinion on this.

I think this problem is entirely something that we as developers see, but
doesn't affect users, and so we have the flexibility to choose how we handle
this.

* Carlos O'Donell:

> On 11/12/19 5:18 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
>>> a problem though. It immediately makes it difficult to test a new glibc on
>>> existing systems without upgrading the system compiler.
>> 
>> I don't think nios2 systems have system compilers as such …
>
> I don't really know what to call the SDK compiler that comes from Altera.
>
> I'm going to call them "system compilers", though I'm open to other terms.

Those are cross-compilers, part of the SDK.  What we call system
compilers are the native compilers installed in /usr.  Typically, they
provide libgcc and libstdc++, but that varies between distributions.

On Tue, 12 Nov 2019, Carlos O'Donell wrote:

> >> How long will that take and is it OK to leave things in a broken
> >> state for that long for a given target like nios2?
> > 
> > Maybe we should back the fix out and see how quickly we can get in the
> > other GCC 10 patches?
> 
> I'd like to hear Joseph's opinion on this.

I think the GCC fix ought to be backported to GCC 8 and 9 branches (and 
generically that applies to fixes relevant to building new glibc versions 
- or to building GCC *with* new glibc versions, sometimes that can justify 
e.g. selective backports of libsanitizer fixes where new glibc broke it).

I don't think we have a basis for backing out this change from glibc at 
this point.  However, if people wish to fix building with GCC 10 on 
previous glibc release branches (and such fixes have been a common class 
of glibc patch backports in the past - evidently some people do wish to 
build glibc release branches with GCC versions that postdate those glibc 
releases), perhaps it would be better for the release-branch fix just to 
disable the warning in the affected file rather than backporting the fix 
from master.

On 11/12/19 3:14 PM, Carlos O'Donell wrote:

> If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
> a problem though. It immediately makes it difficult to test a new glibc on
> existing systems without upgrading the system compiler.
> 
> If there is a backend bug then it absolutely needs to be fixed, and it looks
> like Sandra will try that, but that won't reach the affected compilers until
> much much later.
> 
> I dislike backing out the changes you made because they are quite nice
> cleanups, and the downstream vendors tend to upgrade glibc and gcc and binutils
> all in one big group to avoid these issues so I don't expect they will ever
> see this problem. It's only us as developers that see the larger version
> skew problem.

IIUC Intel uses whatever versions of binutils/gcc/glibc that Mentor 
Graphics gives them (currently 2.32/9.2/2.30 respectively) in their SDK 
and does not mess with mainline trunk sources at all.  I can raise the 
issue with them, but IIRC this would not be the first time some changes 
have been made to glibc that require newish tools for building it.

> My inclination is to fix the gcc problem, backport the solution to the active
> FSF branches, and then that fixes our testers.
> 
> In order of importance:
> - Fix all gcc10 issues.
> - Fix the nios2 backend issue.
> - Backport nios2 backend to active FSF branches.
> - Rebuild and test to make sure we are all clean again.

Yes, that's my plan.

> How long will that take and is it OK to leave things in a broken state for that long
> for a given target like nios2?

Preparing and testing a patch for the Nios II GCC bug might take a 
couple days, but I have another one ahead of it in my queue that I'm 
trying to finish before Stage 1 closes at the end of the week.  So maybe 
a week maximum?

BTW, does this problem affect any other target that tries to use 
GP-relative addressing for small data, like MIPS?

-Sandra

On Tue, 12 Nov 2019, Sandra Loosemore wrote:

> BTW, does this problem affect any other target that tries to use GP-relative
> addressing for small data, like MIPS?

No (that is, no other target is showing such a problem building glibc).

It probably still makes sense for the link test added to the GCC testsuite 
to be target-independent, since it's the sort of bug other targets *could* 
have, and it's quite possible that some non-glibc architectures (of which 
there are a lot in GCC) could have such a bug.

* Sandra Loosemore:

> OK, having a self-contained test case is very helpful.  I'll take a look 
> at fixing this in GCC, although I might not get to it for a few days. 
> Probably the solution is to not use GP-relative addressing for any 
> object declared with a flexible array member.

Thanks for looking into this.  Should I file a GCC PR?

* Joseph Myers:

> On Tue, 12 Nov 2019, Carlos O'Donell wrote:
>
>> >> How long will that take and is it OK to leave things in a broken
>> >> state for that long for a given target like nios2?
>> > 
>> > Maybe we should back the fix out and see how quickly we can get in the
>> > other GCC 10 patches?
>> 
>> I'd like to hear Joseph's opinion on this.
>
> I think the GCC fix ought to be backported to GCC 8 and 9 branches (and 
> generically that applies to fixes relevant to building new glibc versions 
> - or to building GCC *with* new glibc versions, sometimes that can justify 
> e.g. selective backports of libsanitizer fixes where new glibc broke it).
>
> I don't think we have a basis for backing out this change from glibc at 
> this point.  However, if people wish to fix building with GCC 10 on 
> previous glibc release branches (and such fixes have been a common class 
> of glibc patch backports in the past - evidently some people do wish to 
> build glibc release branches with GCC versions that postdate those glibc 
> releases), perhaps it would be better for the release-branch fix just to 
> disable the warning in the affected file rather than backporting the fix 
> from master.

It should be possible to write a configure check for the GCC bug and
build the affected object with -mgpopt=none, since it's a local issue
concentrated to a single object.  Or we can add -mgpopt=none
unconditionally for the time being.

On 11/12/19 10:47 PM, Florian Weimer wrote:
> * Sandra Loosemore:
> 
>> OK, having a self-contained test case is very helpful.  I'll take a look
>> at fixing this in GCC, although I might not get to it for a few days.
>> Probably the solution is to not use GP-relative addressing for any
>> object declared with a flexible array member.
> 
> Thanks for looking into this.  Should I file a GCC PR?

Yes, please.  That will make it easier to track on the GCC side.

-Sandra

On 11/12/19 3:18 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> If the new glibc cannot be compiled with gcc8 or gcc9 for nios2, then that's
>> a problem though. It immediately makes it difficult to test a new glibc on
>> existing systems without upgrading the system compiler.
> 
> I don't think nios2 systems have system compilers as such …
Umm, I've been building nios2 glibc with trunk gcc for about 18 months
without significant issues.  ISTM that if it's broken know it's a recent
issue.


> 
>> How long will that take and is it OK to leave things in a broken
>> state for that long for a given target like nios2?
> 
> Maybe we should back the fix out and see how quickly we can get in the
> other GCC 10 patches?
No strong opinion here.

I can confirm that things look much better after installing Florian's
patches into my tester.  THere's instability due to other GCC issues,
but Florian's code certainly is a step forward.

jeff

On 11/12/19 10:47 PM, Florian Weimer wrote:
> * Sandra Loosemore:
> 
>> OK, having a self-contained test case is very helpful.  I'll take a look 
>> at fixing this in GCC, although I might not get to it for a few days. 
>> Probably the solution is to not use GP-relative addressing for any 
>> object declared with a flexible array member.
> 
> Thanks for looking into this.  Should I file a GCC PR?
Certainly helpful if you do.

Jeff

* Sandra Loosemore:

> On 11/12/19 10:47 PM, Florian Weimer wrote:
>> * Sandra Loosemore:
>> 
>>> OK, having a self-contained test case is very helpful.  I'll take a look
>>> at fixing this in GCC, although I might not get to it for a few days.
>>> Probably the solution is to not use GP-relative addressing for any
>>> object declared with a flexible array member.
>> 
>> Thanks for looking into this.  Should I file a GCC PR?
>
> Yes, please.  That will make it easier to track on the GCC side.

It's PR 92499.  I'm going to post a glibc patch with the workaround
based on the compiler option.