| Message ID | 20190801181534.8802-2-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] Refactor sigcontextinfo.h | expand |
* Adhemerval Zanella: > As indicated by Joseph's comment on BZ#17726, this symbol is most > likely a historical ABI accident. This patch make it on both arm > and sparc ABIs a compat_symbol. > > Checked with a build arm-linux-gnueabihf, sparcv9-linux-gnu, adn > sparc64-linux-gnu to see if the symbol is still present. > > * gmon/Versions (libc) [GLIBC_2.31]: New entry. > * sysdeps/unix/sysv/linux/arm/profil-counter.h (profil_counter): > Make a compat_symbol. > * sysdeps/unix/sysv/linux/sparc/profil-counter.h > (__profil_counter_global): Likewise. There is this bug: <https://sourceware.org/bugzilla/show_bug.cgi?id=14043> So it was already gone for over four years on arm until someone noticed. I wonder if something really needs it? Thanks, Florian
On 02/08/2019 08:09, Florian Weimer wrote: > * Adhemerval Zanella: > >> As indicated by Joseph's comment on BZ#17726, this symbol is most >> likely a historical ABI accident. This patch make it on both arm >> and sparc ABIs a compat_symbol. >> >> Checked with a build arm-linux-gnueabihf, sparcv9-linux-gnu, adn >> sparc64-linux-gnu to see if the symbol is still present. >> >> * gmon/Versions (libc) [GLIBC_2.31]: New entry. >> * sysdeps/unix/sysv/linux/arm/profil-counter.h (profil_counter): >> Make a compat_symbol. >> * sysdeps/unix/sysv/linux/sparc/profil-counter.h >> (__profil_counter_global): Likewise. > > There is this bug: > > <https://sourceware.org/bugzilla/show_bug.cgi?id=14043> > > So it was already gone for over four years on arm until someone noticed. > I wonder if something really needs it? My take on this accidental ABI leakage is just to dropped it, it exposes unnecessary implementation detail, add complexity on arch-specific code base, it is dangerous because pollute application namespace with non expected symbol, and I would consider a potential bug if application are relying on this specific symbol semantic (and it is also non-portable). So I would vote if we just remove it instead of make it a compat symbol.
* Adhemerval Zanella: > On 02/08/2019 08:09, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> As indicated by Joseph's comment on BZ#17726, this symbol is most >>> likely a historical ABI accident. This patch make it on both arm >>> and sparc ABIs a compat_symbol. >>> >>> Checked with a build arm-linux-gnueabihf, sparcv9-linux-gnu, adn >>> sparc64-linux-gnu to see if the symbol is still present. >>> >>> * gmon/Versions (libc) [GLIBC_2.31]: New entry. >>> * sysdeps/unix/sysv/linux/arm/profil-counter.h (profil_counter): >>> Make a compat_symbol. >>> * sysdeps/unix/sysv/linux/sparc/profil-counter.h >>> (__profil_counter_global): Likewise. >> >> There is this bug: >> >> <https://sourceware.org/bugzilla/show_bug.cgi?id=14043> >> >> So it was already gone for over four years on arm until someone noticed. >> I wonder if something really needs it? > > My take on this accidental ABI leakage is just to dropped it, it exposes > unnecessary implementation detail, add complexity on arch-specific code > base, it is dangerous because pollute application namespace with non > expected symbol, and I would consider a potential bug if application are > relying on this specific symbol semantic (and it is also non-portable). > > So I would vote if we just remove it instead of make it a compat symbol. I tend to agree, but I wonder if Joseph filed this bug because it was something that someone actually encountered as the result of a glibc update. In this case, it strongly points to accidental use of the symbol. I'm not necessarily opposed to actually removing symbols from the ABI in general. (I think some of us are though.) I'm wondering if we have evidence that we can't do it here. Thanks, Florian
On Fri, 2 Aug 2019, Florian Weimer wrote: > I tend to agree, but I wonder if Joseph filed this bug because it was > something that someone actually encountered as the result of a glibc > update. In this case, it strongly points to accidental use of the > symbol. I filed the bug because, when we were setting up the ABI test baselines in the first place, it seemed desirable to verify them against binaries built from various past glibc versions (to check as far as possible that the contents of each symbol version in the test expectations were the same as the contents of that version in the corresponding glibc release), and doing such verification showed up that symbol as having been accidentally removed. (I did remove the _q_* symbols from powerpc soft-float; those had, in glibc 2.4, been accidentally added to version GLIBC_2.2, but had never been usefully usable - and made the baseline there reflect the accidental removal of __fe_nomask_env which had also never been usable for that configuration.) -- Joseph S. Myers joseph@codesourcery.com
On 02/08/2019 12:48, Joseph Myers wrote: > On Fri, 2 Aug 2019, Florian Weimer wrote: > >> I tend to agree, but I wonder if Joseph filed this bug because it was >> something that someone actually encountered as the result of a glibc >> update. In this case, it strongly points to accidental use of the >> symbol. > > I filed the bug because, when we were setting up the ABI test baselines in > the first place, it seemed desirable to verify them against binaries built > from various past glibc versions (to check as far as possible that the > contents of each symbol version in the test expectations were the same as > the contents of that version in the corresponding glibc release), and > doing such verification showed up that symbol as having been accidentally > removed. > > (I did remove the _q_* symbols from powerpc soft-float; those had, in > glibc 2.4, been accidentally added to version GLIBC_2.2, but had never > been usefully usable - and made the baseline there reflect the accidental > removal of __fe_nomask_env which had also never been usable for that > configuration.) > So do you think we can remove them or should we keep them? I do think we should just remove these accidental bleeding symbols.
On Fri, 2 Aug 2019, Adhemerval Zanella wrote: > So do you think we can remove them or should we keep them? I do think > we should just remove these accidental bleeding symbols. I think there is a strong presumption in favour of keeping symbols that were public in a (2.1 or later, i.e. after the introduction of symbol versioning) release, in the absence of clear evidence that the symbol would never have been usable. -- Joseph S. Myers joseph@codesourcery.com
On 8/2/19 3:55 PM, Joseph Myers wrote: > On Fri, 2 Aug 2019, Adhemerval Zanella wrote: > >> So do you think we can remove them or should we keep them? I do think >> we should just remove these accidental bleeding symbols. > > I think there is a strong presumption in favour of keeping symbols that > were public in a (2.1 or later, i.e. after the introduction of symbol > versioning) release, in the absence of clear evidence that the symbol > would never have been usable. Agreed. Compat is safe enough. -- Cheers, Carlos.
* Carlos O'Donell: > On 8/2/19 3:55 PM, Joseph Myers wrote: >> On Fri, 2 Aug 2019, Adhemerval Zanella wrote: >> >>> So do you think we can remove them or should we keep them? I do think >>> we should just remove these accidental bleeding symbols. >> >> I think there is a strong presumption in favour of keeping symbols that >> were public in a (2.1 or later, i.e. after the introduction of symbol >> versioning) release, in the absence of clear evidence that the symbol >> would never have been usable. > > Agreed. Compat is safe enough. I think Adhemerval's point is that it adds complexity because it requires support in the tree, for all architecturs, for something that would otherwise be unneeded. If we can avoid that, it's a win for us. Thanks, Florian
On 05/08/2019 06:53, Florian Weimer wrote: > * Carlos O'Donell: > >> On 8/2/19 3:55 PM, Joseph Myers wrote: >>> On Fri, 2 Aug 2019, Adhemerval Zanella wrote: >>> >>>> So do you think we can remove them or should we keep them? I do think >>>> we should just remove these accidental bleeding symbols. >>> >>> I think there is a strong presumption in favour of keeping symbols that >>> were public in a (2.1 or later, i.e. after the introduction of symbol >>> versioning) release, in the absence of clear evidence that the symbol >>> would never have been usable. >> >> Agreed. Compat is safe enough. > > I think Adhemerval's point is that it adds complexity because it > requires support in the tree, for all architecturs, for something that > would otherwise be unneeded. If we can avoid that, it's a win for us. Also that this symbol export was not meant in first place, so it is unlikely programs uses it. And if it were, it likely they either need to reference to glibc implementation to get the correct prototype or it is bleeding in application namespace wrongly, which is most likely.
On Mon, 5 Aug 2019, Adhemerval Zanella wrote:
> Also that this symbol export was not meant in first place, so it is unlikely
Like all other exports, it's listed in a Versions file. So it must have
been deliberate (even if mistaken) at some point.
--
Joseph S. Myers
joseph@codesourcery.com
* Joseph Myers: > On Mon, 5 Aug 2019, Adhemerval Zanella wrote: > >> Also that this symbol export was not meant in first place, so it is unlikely > > Like all other exports, it's listed in a Versions file. So it must have > been deliberate (even if mistaken) at some point. libc.map used to have this: # all functions and variables in the normal name space a*; b*; c*; d*; e*; f*; g*; h*; i*; j*; k*; l*; m*; n*; o*; p*; q*; r*; s*; t*; u*; v*; w*; x*; y*; z*; So the missing static likely triggered the unwanted export, and that mistake was carried over when libc.map was switched over to explicit function names. I do not see evidence of a deliberate decision here.
On 8/5/19 3:41 PM, Florian Weimer wrote: > * Joseph Myers: > >> On Mon, 5 Aug 2019, Adhemerval Zanella wrote: >> >>> Also that this symbol export was not meant in first place, so it is unlikely >> >> Like all other exports, it's listed in a Versions file. So it must have >> been deliberate (even if mistaken) at some point. > > libc.map used to have this: > > # all functions and variables in the normal name space > a*; b*; c*; d*; e*; f*; g*; h*; i*; j*; k*; l*; m*; > n*; o*; p*; q*; r*; s*; t*; u*; v*; w*; x*; y*; z*; > > So the missing static likely triggered the unwanted export, and that > mistake was carried over when libc.map was switched over to explicit > function names. I do not see evidence of a deliberate decision here. This is why we say "strong presumption" :-) We want to err on the side of being cautious unless we have evidence otherwise. If there is no evidence that a deliberate and conscious decision was made, then perhaps simplifying the situation may be fine e.g. no compat symbols. -- Cheers, Carlos.
* Carlos O'Donell: > On 8/5/19 3:41 PM, Florian Weimer wrote: >> * Joseph Myers: >> >>> On Mon, 5 Aug 2019, Adhemerval Zanella wrote: >>> >>>> Also that this symbol export was not meant in first place, so it is unlikely >>> >>> Like all other exports, it's listed in a Versions file. So it must have >>> been deliberate (even if mistaken) at some point. >> >> libc.map used to have this: >> >> # all functions and variables in the normal name space >> a*; b*; c*; d*; e*; f*; g*; h*; i*; j*; k*; l*; m*; >> n*; o*; p*; q*; r*; s*; t*; u*; v*; w*; x*; y*; z*; >> >> So the missing static likely triggered the unwanted export, and that >> mistake was carried over when libc.map was switched over to explicit >> function names. I do not see evidence of a deliberate decision here. > > This is why we say "strong presumption" :-) > > We want to err on the side of being cautious unless we have evidence > otherwise. > > If there is no evidence that a deliberate and conscious decision was > made, then perhaps simplifying the situation may be fine e.g. no compat > symbols. And if necessary, we can add back the symbol again. As far as mistakes go, it wouldn't even be that costly, I think.
On 8/6/19 3:18 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> On 8/5/19 3:41 PM, Florian Weimer wrote: >>> * Joseph Myers: >>> >>>> On Mon, 5 Aug 2019, Adhemerval Zanella wrote: >>>> >>>>> Also that this symbol export was not meant in first place, so it is unlikely >>>> >>>> Like all other exports, it's listed in a Versions file. So it must have >>>> been deliberate (even if mistaken) at some point. >>> >>> libc.map used to have this: >>> >>> # all functions and variables in the normal name space >>> a*; b*; c*; d*; e*; f*; g*; h*; i*; j*; k*; l*; m*; >>> n*; o*; p*; q*; r*; s*; t*; u*; v*; w*; x*; y*; z*; >>> >>> So the missing static likely triggered the unwanted export, and that >>> mistake was carried over when libc.map was switched over to explicit >>> function names. I do not see evidence of a deliberate decision here. >> >> This is why we say "strong presumption" :-) >> >> We want to err on the side of being cautious unless we have evidence >> otherwise. >> >> If there is no evidence that a deliberate and conscious decision was >> made, then perhaps simplifying the situation may be fine e.g. no compat >> symbols. > > And if necessary, we can add back the symbol again. As far as > mistakes go, it wouldn't even be that costly, I think. Just to be clear I have no sustained objection to this change, but also no strong opinion one way or the other. My point is only that I would like to see, what you have provided, which is evidence that this is entirely a historical mistake. I think that if Adhemerval proposes the patch, you review it, and nobody objects, then we should just check it in. As you say we can add a compat symbol later. We know we want to remove it. -- Cheers, Carlos.
On Tue, 6 Aug 2019, Carlos O'Donell wrote: > Just to be clear I have no sustained objection to this change, but also > no strong opinion one way or the other. I am also not making a sustained objection here, just stating the general (rebuttable) presumption that a symbol exported at a public symbol version in a release should be kept even if it was a mistake to export it, in the absence of strong evidence that removing it is safe. -- Joseph S. Myers joseph@codesourcery.com
diff --git a/gmon/Versions b/gmon/Versions index d0b63334f2..cc705bd978 100644 --- a/gmon/Versions +++ b/gmon/Versions @@ -19,4 +19,6 @@ libc { GLIBC_2.2.3 { sprofil; } + GLIBC_2.31 { + } } diff --git a/sysdeps/unix/sysv/linux/arm/profil-counter.h b/sysdeps/unix/sysv/linux/arm/profil-counter.h index 29a9c7fecc..6243a02bb5 100644 --- a/sysdeps/unix/sysv/linux/arm/profil-counter.h +++ b/sysdeps/unix/sysv/linux/arm/profil-counter.h @@ -30,5 +30,8 @@ __profil_counter (int signo, siginfo_t *_si, void *scp) asm volatile (""); } #ifndef __profil_counter -weak_alias (__profil_counter, profil_counter) +# include <shlib-compat.h> +# if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_31) +compat_symbol (libc, __profil_counter, profil_counter, GLIBC_2_0); +# endif #endif diff --git a/sysdeps/unix/sysv/linux/sparc/profil-counter.h b/sysdeps/unix/sysv/linux/sparc/profil-counter.h index ad06a4fe06..01271103bb 100644 --- a/sysdeps/unix/sysv/linux/sparc/profil-counter.h +++ b/sysdeps/unix/sysv/linux/sparc/profil-counter.h @@ -21,6 +21,8 @@ #include <sysdeps/unix/sysv/linux/profil-counter.h> #ifndef __profil_counter +# include <shlib-compat.h> +# if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_31) void __profil_counter_global (int signo, struct sigcontext *si) { @@ -30,5 +32,6 @@ __profil_counter_global (int signo, struct sigcontext *si) profil_count (si->si_regs.pc); #endif } -weak_alias (__profil_counter_global, profil_counter) +compat_symbol (libc, __profil_counter_global, profil_counter, GLIBC_2_0); +# endif #endif