| Message ID | 78f75872-ccdb-4b5e-4e54-f19cb1becbb5@redhat.com |
|---|---|
| State | Committed |
| Headers |
Received: (qmail 69436 invoked by alias); 20 Nov 2018 22:26:41 -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 69417 invoked by uid 89); 20 Nov 2018 22:26:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=Hx-languages-length:2666, consult, weakness, Mathieu X-HELO: mail-qt1-f196.google.com Return-Path: <carlos@redhat.com> To: "Joseph S. Myers" <joseph@codesourcery.com>, GNU C Library <libc-alpha@sourceware.org> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> From: Carlos O'Donell <carlos@redhat.com> Subject: [PATCH] abilist.awk: Treat .tdata like .tbss and reject unknown combinations. Openpgp: preference=signencrypt Message-ID: <78f75872-ccdb-4b5e-4e54-f19cb1becbb5@redhat.com> Date: Tue, 20 Nov 2018 17:26:33 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Carlos O'Donell
Nov. 20, 2018, 10:26 p.m. UTC
abilist.awk: Treat .tdata like .tbss and reject unknown combinations.
Mathieu Desnoyers ran into an issue with his rseq patch where
he was the first person to add weak thread-local data and this
resulted in an ABI list update with entries like this:
"GLIBC_2.29 w ? D .tdata 0000000000000020".
The weakness of the symbol has nothing to do with the DSOs
ABI and so we should not write anything about weak symbols
here. The .tdata entries should be treated exactly like .tbss
entries and the output should have been:
"GLIBC_2.29 __rseq_abi T 0x20"
This change makes abilist.awk handle .tdata just like .tbss, while
at the same time adding an error case for the default. We never
want anyone to be able to add such entries to any ABI list files
and should see an immediate error and consult with experts.
Tested by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
witht the rseq patch set and 'make update-all-abi'.
Tested myself with 'make update-all-abi' on x86_64 with no changes.
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
ChangeLog | 4 ++++
scripts/abilist.awk | 11 +++--------
2 files changed, 7 insertions(+), 8 deletions(-)
Comments
----- On Nov 20, 2018, at 5:26 PM, carlos carlos@redhat.com wrote: > abilist.awk: Treat .tdata like .tbss and reject unknown combinations. > > Mathieu Desnoyers ran into an issue with his rseq patch where > he was the first person to add weak thread-local data and this > resulted in an ABI list update with entries like this: > "GLIBC_2.29 w ? D .tdata 0000000000000020". > > The weakness of the symbol has nothing to do with the DSOs > ABI and so we should not write anything about weak symbols > here. The .tdata entries should be treated exactly like .tbss > entries and the output should have been: > "GLIBC_2.29 __rseq_abi T 0x20" > > This change makes abilist.awk handle .tdata just like .tbss, while > at the same time adding an error case for the default. We never > want anyone to be able to add such entries to any ABI list files > and should see an immediate error and consult with experts. > > Tested by Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > witht the rseq patch set and 'make update-all-abi'. witht -> with Thanks! Mathieu > > Tested myself with 'make update-all-abi' on x86_64 with no changes. > > Signed-off-by: Carlos O'Donell <carlos@redhat.com> > --- > ChangeLog | 4 ++++ > scripts/abilist.awk | 11 +++-------- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 9cdd3bad36..7038b583b6 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,7 @@ > +2018-11-20 Carlos O'Donell <carlos@redhat.com> > + > + * scripts/abilist.awk: Handle .tdata. Error for unknown combinations. > + > 2018-11-20 DJ Delorie <dj@redhat.com> > > * malloc/malloc.c (tcache_entry): Add key field. > diff --git a/scripts/abilist.awk b/scripts/abilist.awk > index bad7c3807e..e914df57f0 100644 > --- a/scripts/abilist.awk > +++ b/scripts/abilist.awk > @@ -39,7 +39,6 @@ $2 == "l" { next } > > # If the target uses ST_OTHER, it will be output before the symbol name. > $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > - weak = $2; > type = $3; > size = $5; > sub(/^0*/, "", size); > @@ -55,7 +54,7 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > if (version == "GLIBC_PRIVATE") next; > > desc = ""; > - if (type == "D" && $4 == ".tbss") { > + if (type == "D" && ($4 == ".tbss" || $4 == ".tdata")) { > type = "T"; > } > else if (type == "D" && $4 == ".opd") { > @@ -90,14 +89,10 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > size = ""; > } > else { > - desc = symbol " " version " " weak " ? " type " " $4 " " $5; > - } > - if (size == " 0x") { > - desc = symbol " " version " " weak " ? " type " " $4 " " $5; > + print "Unable to handle this type of symbol." > + exit 1 > } > > - # Disabled -- weakness should not matter to shared library ABIs any more. > - #if (weak == "w") type = tolower(type); > if (desc == "") > desc = symbol " " type size; > > -- > 2.17.2
On 11/20/18 5:33 PM, Mathieu Desnoyers wrote: > ----- On Nov 20, 2018, at 5:26 PM, carlos carlos@redhat.com wrote: > >> abilist.awk: Treat .tdata like .tbss and reject unknown combinations. >> >> Mathieu Desnoyers ran into an issue with his rseq patch where >> he was the first person to add weak thread-local data and this >> resulted in an ABI list update with entries like this: >> "GLIBC_2.29 w ? D .tdata 0000000000000020". >> >> The weakness of the symbol has nothing to do with the DSOs >> ABI and so we should not write anything about weak symbols >> here. The .tdata entries should be treated exactly like .tbss >> entries and the output should have been: >> "GLIBC_2.29 __rseq_abi T 0x20" >> >> This change makes abilist.awk handle .tdata just like .tbss, while >> at the same time adding an error case for the default. We never >> want anyone to be able to add such entries to any ABI list files >> and should see an immediate error and consult with experts. >> >> Tested by Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >> witht the rseq patch set and 'make update-all-abi'. > > witht -> with Thanks. Fixed (git commit --amend). > Thanks! > > Mathieu > >> >> Tested myself with 'make update-all-abi' on x86_64 with no changes. >> >> Signed-off-by: Carlos O'Donell <carlos@redhat.com> >> --- >> ChangeLog | 4 ++++ >> scripts/abilist.awk | 11 +++-------- >> 2 files changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/ChangeLog b/ChangeLog >> index 9cdd3bad36..7038b583b6 100644 >> --- a/ChangeLog >> +++ b/ChangeLog >> @@ -1,3 +1,7 @@ >> +2018-11-20 Carlos O'Donell <carlos@redhat.com> >> + >> + * scripts/abilist.awk: Handle .tdata. Error for unknown combinations. >> + >> 2018-11-20 DJ Delorie <dj@redhat.com> >> >> * malloc/malloc.c (tcache_entry): Add key field. >> diff --git a/scripts/abilist.awk b/scripts/abilist.awk >> index bad7c3807e..e914df57f0 100644 >> --- a/scripts/abilist.awk >> +++ b/scripts/abilist.awk >> @@ -39,7 +39,6 @@ $2 == "l" { next } >> >> # If the target uses ST_OTHER, it will be output before the symbol name. >> $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> - weak = $2; >> type = $3; >> size = $5; >> sub(/^0*/, "", size); >> @@ -55,7 +54,7 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> if (version == "GLIBC_PRIVATE") next; >> >> desc = ""; >> - if (type == "D" && $4 == ".tbss") { >> + if (type == "D" && ($4 == ".tbss" || $4 == ".tdata")) { >> type = "T"; >> } >> else if (type == "D" && $4 == ".opd") { >> @@ -90,14 +89,10 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> size = ""; >> } >> else { >> - desc = symbol " " version " " weak " ? " type " " $4 " " $5; >> - } >> - if (size == " 0x") { >> - desc = symbol " " version " " weak " ? " type " " $4 " " $5; >> + print "Unable to handle this type of symbol." >> + exit 1 >> } >> >> - # Disabled -- weakness should not matter to shared library ABIs any more. >> - #if (weak == "w") type = tolower(type); >> if (desc == "") >> desc = symbol " " type size; >> >> -- >> 2.17.2 >
On Nov 20 2018, Carlos O'Donell <carlos@redhat.com> wrote: > +2018-11-20 Carlos O'Donell <carlos@redhat.com> > + > + * scripts/abilist.awk: Handle .tdata. Error for unknown combinations. > + > 2018-11-20 DJ Delorie <dj@redhat.com> > > * malloc/malloc.c (tcache_entry): Add key field. > diff --git a/scripts/abilist.awk b/scripts/abilist.awk > index bad7c3807e..e914df57f0 100644 > --- a/scripts/abilist.awk > +++ b/scripts/abilist.awk > @@ -39,7 +39,6 @@ $2 == "l" { next } > > # If the target uses ST_OTHER, it will be output before the symbol name. > $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > - weak = $2; > type = $3; > size = $5; > sub(/^0*/, "", size); > @@ -55,7 +54,7 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > if (version == "GLIBC_PRIVATE") next; > > desc = ""; > - if (type == "D" && $4 == ".tbss") { > + if (type == "D" && ($4 == ".tbss" || $4 == ".tdata")) { > type = "T"; > } > else if (type == "D" && $4 == ".opd") { > @@ -90,14 +89,10 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > size = ""; > } > else { > - desc = symbol " " version " " weak " ? " type " " $4 " " $5; > - } > - if (size == " 0x") { > - desc = symbol " " version " " weak " ? " type " " $4 " " $5; > + print "Unable to handle this type of symbol." > + exit 1 Perhaps also exit here when seeing an unrecognized line? { print "Don't grok this line:", $0 } Andreas.
On 11/21/18 4:19 AM, Andreas Schwab wrote: > On Nov 20 2018, Carlos O'Donell <carlos@redhat.com> wrote: > >> +2018-11-20 Carlos O'Donell <carlos@redhat.com> >> + >> + * scripts/abilist.awk: Handle .tdata. Error for unknown combinations. >> + >> 2018-11-20 DJ Delorie <dj@redhat.com> >> >> * malloc/malloc.c (tcache_entry): Add key field. >> diff --git a/scripts/abilist.awk b/scripts/abilist.awk >> index bad7c3807e..e914df57f0 100644 >> --- a/scripts/abilist.awk >> +++ b/scripts/abilist.awk >> @@ -39,7 +39,6 @@ $2 == "l" { next } >> >> # If the target uses ST_OTHER, it will be output before the symbol name. >> $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> - weak = $2; >> type = $3; >> size = $5; >> sub(/^0*/, "", size); >> @@ -55,7 +54,7 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> if (version == "GLIBC_PRIVATE") next; >> >> desc = ""; >> - if (type == "D" && $4 == ".tbss") { >> + if (type == "D" && ($4 == ".tbss" || $4 == ".tdata")) { >> type = "T"; >> } >> else if (type == "D" && $4 == ".opd") { >> @@ -90,14 +89,10 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { >> size = ""; >> } >> else { >> - desc = symbol " " version " " weak " ? " type " " $4 " " $5; >> - } >> - if (size == " 0x") { >> - desc = symbol " " version " " weak " ? " type " " $4 " " $5; >> + print "Unable to handle this type of symbol." >> + exit 1 > > Perhaps also exit here when seeing an unrecognized line? > > { > print "Don't grok this line:", $0 > } Good suggestion. We should absolutely do that. Done. I've committed the results after re-testing on x86_64.
I'm now seeing compilation tests failing for i686-gnu.
LC_ALL=C gawk -f ../scripts/abilist.awk /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.dynsym > /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlistT
../Makerules:1344: recipe for target '/scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlist' failed
make[3]: *** [/scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlist] Error 1
These tests for libmachuser and libhurduser are XFAILed in
sysdeps/mach/hurd/i386/Makefile, but that's for the comparison failing,
whereas now the generation of the .symlist file fails ("ERROR: Unable to
handle this type of symbol." - it would be helpful for that message to say
exactly what the line is that generates the error) and so "make check"
exits early.
On 11/27/18 11:42 AM, Joseph Myers wrote: > I'm now seeing compilation tests failing for i686-gnu. > > LC_ALL=C gawk -f ../scripts/abilist.awk /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.dynsym > /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlistT > ../Makerules:1344: recipe for target '/scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlist' failed > make[3]: *** [/scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/mach/libmachuser.symlist] Error 1 > > These tests for libmachuser and libhurduser are XFAILed in > sysdeps/mach/hurd/i386/Makefile, but that's for the comparison failing, > whereas now the generation of the .symlist file fails ("ERROR: Unable to > handle this type of symbol." - it would be helpful for that message to say > exactly what the line is that generates the error) and so "make check" > exits early. I did not run a build-many-glibc's test for this change, thinking that it wouldn't impact any other machine/OS combinations. I have just started a build-many-glibc's run to see if anything else fails. I'll add some debugging code and see if I can see what's wrong.
diff --git a/ChangeLog b/ChangeLog index 9cdd3bad36..7038b583b6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2018-11-20 Carlos O'Donell <carlos@redhat.com> + + * scripts/abilist.awk: Handle .tdata. Error for unknown combinations. + 2018-11-20 DJ Delorie <dj@redhat.com> * malloc/malloc.c (tcache_entry): Add key field. diff --git a/scripts/abilist.awk b/scripts/abilist.awk index bad7c3807e..e914df57f0 100644 --- a/scripts/abilist.awk +++ b/scripts/abilist.awk @@ -39,7 +39,6 @@ $2 == "l" { next } # If the target uses ST_OTHER, it will be output before the symbol name. $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { - weak = $2; type = $3; size = $5; sub(/^0*/, "", size); @@ -55,7 +54,7 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { if (version == "GLIBC_PRIVATE") next; desc = ""; - if (type == "D" && $4 == ".tbss") { + if (type == "D" && ($4 == ".tbss" || $4 == ".tdata")) { type = "T"; } else if (type == "D" && $4 == ".opd") { @@ -90,14 +89,10 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { size = ""; } else { - desc = symbol " " version " " weak " ? " type " " $4 " " $5; - } - if (size == " 0x") { - desc = symbol " " version " " weak " ? " type " " $4 " " $5; + print "Unable to handle this type of symbol." + exit 1 } - # Disabled -- weakness should not matter to shared library ABIs any more. - #if (weak == "w") type = tolower(type); if (desc == "") desc = symbol " " type size;