RFC: Remove --disable-hidden-plt

RFC: Remove --disable-hidden-plt

[H.J. Lu]

--disable-hidden-plt was added by

commit 749a9a4fbfd261e2d9811d9bc4507478c2f7cc58
Author: Roland McGrath <roland@gnu.org>
Date:   Tue Oct 1 08:45:44 2002 +0000

    2002-10-01  Roland McGrath  <roland@frob.com>

            * config.h.in (NO_HIDDEN): New #undef.
            * include/libc-symbols.h [! NO_HIDDEN]: Add this condition to
            nonempty definitions of hidden_proto et al.
            * configure.in: Grok --disable-hidden-plt to define NO_HIDDEN.
            * configure: Regenerated.
            * sysdeps/mach/hurd/configure.in: Always define NO_HIDDEN.
            * sysdeps/mach/hurd/configure: Regenerated.

It doesn't work on x86-64:

https://sourceware.org/bugzilla/show_bug.cgi?id=27692

Should it be removed?

-- 
H.J.

Re: RFC: Remove --disable-hidden-plt

[Adhemerval Zanella]

On 03/04/2021 14:41, H.J. Lu via Libc-alpha wrote:
> --disable-hidden-plt was added by
> 
> commit 749a9a4fbfd261e2d9811d9bc4507478c2f7cc58
> Author: Roland McGrath <roland@gnu.org>
> Date:   Tue Oct 1 08:45:44 2002 +0000
> 
>     2002-10-01  Roland McGrath  <roland@frob.com>
> 
>             * config.h.in (NO_HIDDEN): New #undef.
>             * include/libc-symbols.h [! NO_HIDDEN]: Add this condition to
>             nonempty definitions of hidden_proto et al.
>             * configure.in: Grok --disable-hidden-plt to define NO_HIDDEN.
>             * configure: Regenerated.
>             * sysdeps/mach/hurd/configure.in: Always define NO_HIDDEN.
>             * sysdeps/mach/hurd/configure: Regenerated.
> 
> It doesn't work on x86-64:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=27692
> 
> Should it be removed?
> 

My take is to remove it.  Besides the reasons noted by Szabolcs [1],
some symbols interposition are not really possible on some scenarios
(for instance intra IFUNC call through hidden symbol on i686 and ppc32),
it requires more development effort (as any configure switch and we
will probably need a configure check to see if *all* supposed intra
calls are actually being called through PLT and fix bugs with missing
support), there is no clear advantage of providing such functionality, 
and it broken on every architecture for some time (at least since
2.27).

However it seems it will still need to support it internally for
Hurd and it does catch a potential issue where some implementation
uses a non intended interface (such as __hidden_ver1).

I tried to understand why Roland's has added it in the first place,
but there is no discussion about it on libc-alpha in Sep/Oct 2002.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27692#c7

Re: RFC: Remove --disable-hidden-plt

[Samuel Thibault]

Adhemerval Zanella via Libc-alpha, le mar. 06 avril 2021 14:12:41 -0300, a ecrit:
> 
> 
> On 03/04/2021 14:41, H.J. Lu via Libc-alpha wrote:
> > --disable-hidden-plt was added by
> > 
> > commit 749a9a4fbfd261e2d9811d9bc4507478c2f7cc58
> > Author: Roland McGrath <roland@gnu.org>
> > Date:   Tue Oct 1 08:45:44 2002 +0000
> > 
> >     2002-10-01  Roland McGrath  <roland@frob.com>
> > 
> >             * config.h.in (NO_HIDDEN): New #undef.
> >             * include/libc-symbols.h [! NO_HIDDEN]: Add this condition to
> >             nonempty definitions of hidden_proto et al.
> >             * configure.in: Grok --disable-hidden-plt to define NO_HIDDEN.
> >             * configure: Regenerated.
> >             * sysdeps/mach/hurd/configure.in: Always define NO_HIDDEN.
> >             * sysdeps/mach/hurd/configure: Regenerated.
> > 
> > It doesn't work on x86-64:
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=27692
> > 
> > Should it be removed?
> > 
> 
> My take is to remove it.  Besides the reasons noted by Szabolcs [1],
> some symbols interposition are not really possible on some scenarios
> (for instance intra IFUNC call through hidden symbol on i686 and ppc32),
> it requires more development effort (as any configure switch and we
> will probably need a configure check to see if *all* supposed intra
> calls are actually being called through PLT and fix bugs with missing
> support), there is no clear advantage of providing such functionality, 
> and it broken on every architecture for some time (at least since
> 2.27).
> 
> However it seems it will still need to support it internally for
> Hurd

Yes, some symbols in ld.so (most of sysdeps/mach/hurd/dl-sysdep.c)
*need* to be interposed by the loaded libc.so, because the
implementation in ld.so is to be used only at bootstrap of the program,
and then overriden by the real full-fledged implementation.

Samuel

> and it does catch a potential issue where some implementation
> uses a non intended interface (such as __hidden_ver1).
> 
> I tried to understand why Roland's has added it in the first place,
> but there is no discussion about it on libc-alpha in Sep/Oct 2002.
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=27692#c7

Re: RFC: Remove --disable-hidden-plt

[Adhemerval Zanella]

On 06/04/2021 14:18, Samuel Thibault wrote:
> Adhemerval Zanella via Libc-alpha, le mar. 06 avril 2021 14:12:41 -0300, a ecrit:
>>
>>
>> On 03/04/2021 14:41, H.J. Lu via Libc-alpha wrote:
>>> --disable-hidden-plt was added by
>>>
>>> commit 749a9a4fbfd261e2d9811d9bc4507478c2f7cc58
>>> Author: Roland McGrath <roland@gnu.org>
>>> Date:   Tue Oct 1 08:45:44 2002 +0000
>>>
>>>     2002-10-01  Roland McGrath  <roland@frob.com>
>>>
>>>             * config.h.in (NO_HIDDEN): New #undef.
>>>             * include/libc-symbols.h [! NO_HIDDEN]: Add this condition to
>>>             nonempty definitions of hidden_proto et al.
>>>             * configure.in: Grok --disable-hidden-plt to define NO_HIDDEN.
>>>             * configure: Regenerated.
>>>             * sysdeps/mach/hurd/configure.in: Always define NO_HIDDEN.
>>>             * sysdeps/mach/hurd/configure: Regenerated.
>>>
>>> It doesn't work on x86-64:
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=27692
>>>
>>> Should it be removed?
>>>
>>
>> My take is to remove it.  Besides the reasons noted by Szabolcs [1],
>> some symbols interposition are not really possible on some scenarios
>> (for instance intra IFUNC call through hidden symbol on i686 and ppc32),
>> it requires more development effort (as any configure switch and we
>> will probably need a configure check to see if *all* supposed intra
>> calls are actually being called through PLT and fix bugs with missing
>> support), there is no clear advantage of providing such functionality, 
>> and it broken on every architecture for some time (at least since
>> 2.27).
>>
>> However it seems it will still need to support it internally for
>> Hurd
> 
> Yes, some symbols in ld.so (most of sysdeps/mach/hurd/dl-sysdep.c)
> *need* to be interposed by the loaded libc.so, because the
> implementation in ld.so is to be used only at bootstrap of the program,
> and then overriden by the real full-fledged implementation.

Are these the only ones required to Hurd? I wonder if would be simple to
instead of have the bit NO_HIDDEN hammer, to create a different
hidden_def/hidden_proto to be used on the required symbols (where default
version would map to use non-PLT call and Hurd could just reimplement
it).

Re: RFC: Remove --disable-hidden-plt

[Samuel Thibault]

Adhemerval Zanella, le mar. 06 avril 2021 14:39:27 -0300, a ecrit:
> On 06/04/2021 14:18, Samuel Thibault wrote:
> > Adhemerval Zanella via Libc-alpha, le mar. 06 avril 2021 14:12:41 -0300, a ecrit:
> >> On 03/04/2021 14:41, H.J. Lu via Libc-alpha wrote:
> >> However it seems it will still need to support it internally for
> >> Hurd
> > 
> > Yes, some symbols in ld.so (most of sysdeps/mach/hurd/dl-sysdep.c)
> > *need* to be interposed by the loaded libc.so, because the
> > implementation in ld.so is to be used only at bootstrap of the program,
> > and then overriden by the real full-fledged implementation.
> 
> Are these the only ones required to Hurd?

Yes. Previously we were using the NO_HIDDEN hammer, but:

> I wonder if would be simple to instead of have the bit NO_HIDDEN
> hammer, to create a different hidden_def/hidden_proto to be used on
> the required symbols (where default version would map to use non-PLT
> call and Hurd could just reimplement it).

That was already roughly done in 
3d1870fa3301c5cd00d5fdab0014c4e22b71fef2
where we made all rtld_hidden_proto respect NO_RTLD_HIDDEN.

I didn't specifically mark the exactly required functions, but the
localplt check exceptions in sysdeps/mach/hurd/i386/localplt.data is
already exactly the list of needed PLTs, so I don't think it's worth
the extra maintenance complexity of special-casing in the headers for
everyone in addition to the rtld implementation in dl-sysdep.c.

Samuel

Re: RFC: Remove --disable-hidden-plt

[Adhemerval Zanella]

On 06/04/2021 15:03, Samuel Thibault wrote:
> Adhemerval Zanella, le mar. 06 avril 2021 14:39:27 -0300, a ecrit:
>> On 06/04/2021 14:18, Samuel Thibault wrote:
>>> Adhemerval Zanella via Libc-alpha, le mar. 06 avril 2021 14:12:41 -0300, a ecrit:
>>>> On 03/04/2021 14:41, H.J. Lu via Libc-alpha wrote:
>>>> However it seems it will still need to support it internally for
>>>> Hurd
>>>
>>> Yes, some symbols in ld.so (most of sysdeps/mach/hurd/dl-sysdep.c)
>>> *need* to be interposed by the loaded libc.so, because the
>>> implementation in ld.so is to be used only at bootstrap of the program,
>>> and then overriden by the real full-fledged implementation.
>>
>> Are these the only ones required to Hurd?
> 
> Yes. Previously we were using the NO_HIDDEN hammer, but:
> 
>> I wonder if would be simple to instead of have the bit NO_HIDDEN
>> hammer, to create a different hidden_def/hidden_proto to be used on
>> the required symbols (where default version would map to use non-PLT
>> call and Hurd could just reimplement it).
> 
> That was already roughly done in 
> 3d1870fa3301c5cd00d5fdab0014c4e22b71fef2
> where we made all rtld_hidden_proto respect NO_RTLD_HIDDEN.
> 
> I didn't specifically mark the exactly required functions, but the
> localplt check exceptions in sysdeps/mach/hurd/i386/localplt.data is
> already exactly the list of needed PLTs, so I don't think it's worth
> the extra maintenance complexity of special-casing in the headers for
> everyone in addition to the rtld implementation in dl-sysdep.c.

Currently NO_RTLD_HIDDEN spills on some generic code:

config.h.in:#undef      NO_RTLD_HIDDEN
elf/dl-minimal.c:# ifndef NO_RTLD_HIDDEN
elf/dl-minimal.c:# ifndef NO_RTLD_HIDDEN
include/assert.h:# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
include/sys/stat.h:# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
include/unistd.h:#  ifndef NO_RTLD_HIDDEN

With the removal of NO_HIDDEN configure support, I would like to
move NO_RTLD_HIDDEN to be Hurd specific if possible.

Re: RFC: Remove --disable-hidden-plt

[Samuel Thibault]

Adhemerval Zanella, le mar. 06 avril 2021 15:15:01 -0300, a ecrit:
> Currently NO_RTLD_HIDDEN spills on some generic code:
> 
> config.h.in:#undef      NO_RTLD_HIDDEN
> elf/dl-minimal.c:# ifndef NO_RTLD_HIDDEN
> elf/dl-minimal.c:# ifndef NO_RTLD_HIDDEN
> include/assert.h:# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
> include/sys/stat.h:# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
> include/unistd.h:#  ifndef NO_RTLD_HIDDEN

Ah, so actually I eventually did the work of marking the specific
functions. That's the extra maintenance complexity I was talking about.

> With the removal of NO_HIDDEN configure support, I would like to
> move NO_RTLD_HIDDEN to be Hurd specific

But we don't want to duplicate code either.

> if possible.

It's always possible to duplicate the code but that'd be forking
maintenance. We could split the few NO_RTLD_HIDDEN lines in a separate
header that sysdeps/mach/hurd/ could override with an empty file? It
does not seem to me really making the code more readable than the
ifndef...

Samuel