[2/2] linux: Make profil_counter a compat_symbol (BZ#17726)
diff mbox series

Message ID 20190801181534.8802-2-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [v2,1/2] Refactor sigcontextinfo.h
Related show

Commit Message

Adhemerval Zanella Aug. 1, 2019, 6:15 p.m. UTC
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.
---
 gmon/Versions                                  | 2 ++
 sysdeps/unix/sysv/linux/arm/profil-counter.h   | 5 ++++-
 sysdeps/unix/sysv/linux/sparc/profil-counter.h | 5 ++++-
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Florian Weimer Aug. 2, 2019, 11:09 a.m. UTC | #1
* 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
Adhemerval Zanella Aug. 2, 2019, 12:30 p.m. UTC | #2
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.
Florian Weimer Aug. 2, 2019, 12:34 p.m. UTC | #3
* 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
Joseph Myers Aug. 2, 2019, 3:48 p.m. UTC | #4
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.)
Adhemerval Zanella Aug. 2, 2019, 7:23 p.m. UTC | #5
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.
Joseph Myers Aug. 2, 2019, 7:55 p.m. UTC | #6
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.
Carlos O'Donell Aug. 2, 2019, 8:13 p.m. UTC | #7
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.
Florian Weimer Aug. 5, 2019, 9:53 a.m. UTC | #8
* 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
Adhemerval Zanella Aug. 5, 2019, 11:47 a.m. UTC | #9
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.
Joseph Myers Aug. 5, 2019, 7:28 p.m. UTC | #10
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.
Florian Weimer Aug. 5, 2019, 7:41 p.m. UTC | #11
* 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.
Carlos O'Donell Aug. 6, 2019, 7:08 p.m. UTC | #12
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.
Florian Weimer Aug. 6, 2019, 7:18 p.m. UTC | #13
* 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.
Carlos O'Donell Aug. 6, 2019, 7:22 p.m. UTC | #14
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.
Joseph Myers Aug. 6, 2019, 8:35 p.m. UTC | #15
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.

Patch
diff mbox series

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