Message ID | gerrit.1572801105000.I51be146a7857186a4ede0bb40b332509487bdde8@gnutoolchain-gerrit.osci.io |
---|---|
State | Superseded |
Headers |
Received: (qmail 2836 invoked by alias); 3 Nov 2019 17:11:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 2691 invoked by uid 89); 3 Nov 2019 17:11:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN autolearn=ham version=3.3.1 spammy=1037 X-HELO: mx1.osci.io X-Gerrit-PatchSet: 1 Date: Sun, 3 Nov 2019 12:11:46 -0500 From: "Florian Weimer (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io> To: libc-alpha@sourceware.org Cc: Florian Weimer <fweimer@redhat.com> Message-ID: <gerrit.1572801105000.I51be146a7857186a4ede0bb40b332509487bdde8@gnutoolchain-gerrit.osci.io> Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] slotinfo in struct dtv_slotinfo_list should be flexible array [BZ #25... X-Gerrit-Change-Id: I51be146a7857186a4ede0bb40b332509487bdde8 X-Gerrit-Change-Number: 489 X-Gerrit-ChangeURL: <https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/489> X-Gerrit-Commit: 7591046cb1bf663525ed73229ab1ec03eb756326 References: <gerrit.1572801105000.I51be146a7857186a4ede0bb40b332509487bdde8@gnutoolchain-gerrit.osci.io> Reply-To: fweimer@redhat.com, fweimer@redhat.com, libc-alpha@sourceware.org MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 |
Commit Message
Simon Marchi (Code Review)
Nov. 3, 2019, 5:11 p.m. UTC
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(-)
Comments
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.
diff --git a/csu/libc-tls.c b/csu/libc-tls.c index 33d8b61..23d6cde 100644 --- a/csu/libc-tls.c +++ b/csu/libc-tls.c @@ -23,7 +23,7 @@ #include <unistd.h> #include <stdio.h> #include <sys/param.h> - +#include <array_length.h> #ifdef SHARED #error makefile bug, this file is for static only @@ -32,17 +32,11 @@ dtv_t _dl_static_dtv[2 + TLS_SLOTINFO_SURPLUS]; -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] = { } }, + }; /* Highest dtv index currently needed. */ size_t _dl_tls_max_dtv_idx; @@ -72,16 +66,16 @@ 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. */ /* The slotinfo list. Will be extended by the code doing dynamic linking. */ GL(dl_tls_max_dtv_idx) = 1; - GL(dl_tls_dtv_slotinfo_list) = &static_slotinfo.si; + GL(dl_tls_dtv_slotinfo_list) = &static_slotinfo; } static void @@ -206,7 +200,7 @@ init_slotinfo (); // static_slotinfo.si.slotinfo[1].gen = 0; already zero - static_slotinfo.si.slotinfo[1].map = main_map; + static_slotinfo.slotinfo[1].map = main_map; memsz = roundup (memsz, align ?: 1); diff --git a/nptl_db/db-symbols.h b/nptl_db/db-symbols.h index 8b078b0..7c53d80 100644 --- a/nptl_db/db-symbols.h +++ b/nptl_db/db-symbols.h @@ -25,6 +25,7 @@ DB_LOOKUP_NAME (SYM_SIZEOF_##type, _thread_db_sizeof_##type) #define DB_STRUCT_FIELD(type, field) \ DB_LOOKUP_NAME (SYM_##type##_FIELD_##field, _thread_db_##type##_##field) +#define DB_STRUCT_FLEXIBLE_ARRAY(type, field) DB_STRUCT_FIELD (type, field) #define DB_SYMBOL(name) \ DB_LOOKUP_NAME (SYM_##name, name) #define DB_FUNCTION(name) \ @@ -36,6 +37,8 @@ # include "structs.def" # undef DB_STRUCT +# undef DB_STRUCT_FIELD +# undef DB_STRUCT_FLEXIBLE_ARRAY # undef DB_FUNCTION # undef DB_SYMBOL # undef DB_VARIABLE diff --git a/nptl_db/db_info.c b/nptl_db/db_info.c index af7f754..5a4baa8 100644 --- a/nptl_db/db_info.c +++ b/nptl_db/db_info.c @@ -54,8 +54,11 @@ 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], \ 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 @@ -77,6 +80,9 @@ #define DB_STRUCT_ARRAY_FIELD(type, field) \ ARRAY_DESC (_thread_db_##type##_##field, \ offsetof (type, field), ((type *) 0)->field) +#define DB_STRUCT_FLEXIBLE_ARRAY(type, field) \ + FLEXIBLE_ARRAY_DESC (_thread_db_##type##_##field, \ + offsetof (type, field), ((type *) 0)->field) #define DB_VARIABLE(name) DESC (_thread_db_##name, 0, name) #define DB_ARRAY_VARIABLE(name) ARRAY_DESC (_thread_db_##name, 0, name) #define DB_SYMBOL(name) /* Nothing. */ diff --git a/nptl_db/structs.def b/nptl_db/structs.def index f834d1d..a3aa71a 100644 --- a/nptl_db/structs.def +++ b/nptl_db/structs.def @@ -110,7 +110,7 @@ DB_STRUCT (dtv_slotinfo_list) DB_STRUCT_FIELD (dtv_slotinfo_list, len) DB_STRUCT_FIELD (dtv_slotinfo_list, next) -DB_STRUCT_ARRAY_FIELD (dtv_slotinfo_list, slotinfo) +DB_STRUCT_FLEXIBLE_ARRAY (dtv_slotinfo_list, slotinfo) DB_STRUCT (dtv_slotinfo) DB_STRUCT_FIELD (dtv_slotinfo, gen) diff --git a/nptl_db/thread_dbP.h b/nptl_db/thread_dbP.h index 1dbb543..cf6a2b0 100644 --- a/nptl_db/thread_dbP.h +++ b/nptl_db/thread_dbP.h @@ -37,12 +37,14 @@ { # define DB_STRUCT(type) SYM_SIZEOF_##type, # define DB_STRUCT_FIELD(type, field) SYM_##type##_FIELD_##field, +# define DB_STRUCT_FLEXIBLE_ARRAY(type, field) DB_STRUCT_FIELD (type, field) # define DB_SYMBOL(name) SYM_##name, # define DB_FUNCTION(name) SYM_##name, # define DB_VARIABLE(name) SYM_##name, SYM_DESC_##name, # include "structs.def" # undef DB_STRUCT # undef DB_STRUCT_FIELD +# undef DB_STRUCT_FLEXIBLE_ARRAY # undef DB_SYMBOL # undef DB_FUNCTION # undef DB_VARIABLE @@ -90,6 +92,7 @@ uint32_t ta_sizeof_##type; # define DB_STRUCT_FIELD(type, field) \ db_desc_t ta_field_##type##_##field; +# define DB_STRUCT_FLEXIBLE_ARRAY(type, field) DB_STRUCT_FIELD (type, field) # define DB_SYMBOL(name) \ psaddr_t ta_addr_##name; # define DB_FUNCTION(name) \ @@ -100,6 +103,7 @@ # include "structs.def" # undef DB_STRUCT # undef DB_STRUCT_FIELD +# undef DB_STRUCT_FLEXIBLE_ARRAY # undef DB_FUNCTION # undef DB_SYMBOL # undef DB_VARIABLE diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index eb6cbea..4d67c05 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -421,7 +421,7 @@ { size_t gen; struct link_map *map; - } slotinfo[0]; + } slotinfo[]; } *_dl_tls_dtv_slotinfo_list; /* Number of modules in the static TLS block. */ EXTERN size_t _dl_tls_static_nelem;