diff mbox series

[v2] <shlib-compat.h>: Support compat_symbol_reference for _ISOMAC

Message ID 87y2f5wdy5.fsf@oldenburg.str.redhat.com
State Superseded
Headers
Series [v2] <shlib-compat.h>: Support compat_symbol_reference for _ISOMAC |

Commit Message

Florian Weimer March 2, 2021, 7:30 p.m. UTC 
  This is helpful for testing compat symbols in cases where _ISOMAC
is activated implicitly due to -DMODULE_NAME=testsuite and cannot
be disabled easily.

---
v2: Now actually tested in the situation where I need it.

 include/libc-symbols.h | 28 ++++++++++++++--------------
 include/shlib-compat.h |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

Comments

H.J. Lu March 2, 2021, 8:44 p.m. UTC | #1 
On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> This is helpful for testing compat symbols in cases where _ISOMAC
> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
> be disabled easily.
>
> ---
> v2: Now actually tested in the situation where I need it.
>

Any particular tests which need this?
Florian Weimer March 2, 2021, 9:02 p.m. UTC | #2 
* H. J. Lu:

> On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> This is helpful for testing compat symbols in cases where _ISOMAC
>> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
>> be disabled easily.
>>
>> ---
>> v2: Now actually tested in the situation where I need it.
>>
>
> Any particular tests which need this?

nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
_pthread_cleanup_pop are compatibility symbols.

Thanks,
Florian
H.J. Lu March 2, 2021, 9:18 p.m. UTC | #3 
On Tue, Mar 2, 2021 at 1:02 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> This is helpful for testing compat symbols in cases where _ISOMAC
> >> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
> >> be disabled easily.
> >>
> >> ---
> >> v2: Now actually tested in the situation where I need it.
> >>
> >
> > Any particular tests which need this?
>
> nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
> _pthread_cleanup_pop are compatibility symbols.
>

Do you just want to link against the older version and nothing else?
H.J. Lu March 2, 2021, 9:23 p.m. UTC | #4 
On Tue, Mar 2, 2021 at 1:18 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Mar 2, 2021 at 1:02 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * H. J. Lu:
> >
> > > On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
> > > <libc-alpha@sourceware.org> wrote:
> > >>
> > >> This is helpful for testing compat symbols in cases where _ISOMAC
> > >> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
> > >> be disabled easily.
> > >>
> > >> ---
> > >> v2: Now actually tested in the situation where I need it.
> > >>
> > >
> > > Any particular tests which need this?
> >
> > nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
> > _pthread_cleanup_pop are compatibility symbols.
> >
>
> Do you just want to link against the older version and nothing else?
>

Why are these tests OK without your patch?

malloc/tst-mallocstate.c:compat_symbol_reference (libc,
malloc_get_state, malloc_get_state, GLIBC_2_0);
malloc/tst-mallocstate.c:compat_symbol_reference (libc,
malloc_set_state, malloc_set_state, GLIBC_2_0);
posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
glob, GLIBC_2_1);
posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
glob, GLIBC_2_0);
posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawn,
posix_spawn, GLIBC_2_2);
posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawnp,
posix_spawnp, GLIBC_2_2);
resolv/tst-p_secstodate.c:compat_symbol_reference (libresolv,
__p_secstodate, __p_secstodate, GLIBC_2_0);
sunrpc/tst-svc_register.c:compat_symbol_reference (libc, xdr_pmap,
xdr_pmap, GLIBC_2_0);
sunrpc/tst-svc_register.c:compat_symbol_reference (libc,
svc_unregister, svc_unregister, GLIBC_2_0);
sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c:compat_symbol_reference
(libc, fcntl, fcntl, GLIBC_2_0);
sysdeps/unix/sysv/linux/tst-readdir64-compat.c:compat_symbol_reference
(libc, compat_readdir64, readdir64, GLIBC_2_1);
Florian Weimer March 2, 2021, 9:26 p.m. UTC | #5 
* H. J. Lu:

> On Tue, Mar 2, 2021 at 1:02 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
>> > <libc-alpha@sourceware.org> wrote:
>> >>
>> >> This is helpful for testing compat symbols in cases where _ISOMAC
>> >> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
>> >> be disabled easily.
>> >>
>> >> ---
>> >> v2: Now actually tested in the situation where I need it.
>> >>
>> >
>> > Any particular tests which need this?
>>
>> nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
>> _pthread_cleanup_pop are compatibility symbols.

> Do you just want to link against the older version and nothing else?

It's the only version, but yes.

Thanks,
Florian
Florian Weimer March 2, 2021, 9:28 p.m. UTC | #6 
* H. J. Lu:

> On Tue, Mar 2, 2021 at 1:18 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Mar 2, 2021 at 1:02 PM Florian Weimer <fweimer@redhat.com> wrote:
>> >
>> > * H. J. Lu:
>> >
>> > > On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
>> > > <libc-alpha@sourceware.org> wrote:
>> > >>
>> > >> This is helpful for testing compat symbols in cases where _ISOMAC
>> > >> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
>> > >> be disabled easily.
>> > >>
>> > >> ---
>> > >> v2: Now actually tested in the situation where I need it.
>> > >>
>> > >
>> > > Any particular tests which need this?
>> >
>> > nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
>> > _pthread_cleanup_pop are compatibility symbols.
>> >
>>
>> Do you just want to link against the older version and nothing else?
>>
>
> Why are these tests OK without your patch?
>
> malloc/tst-mallocstate.c:compat_symbol_reference (libc,
> malloc_get_state, malloc_get_state, GLIBC_2_0);
> malloc/tst-mallocstate.c:compat_symbol_reference (libc,
> malloc_set_state, malloc_set_state, GLIBC_2_0);

tests-internal := tst-mallocstate tst-scratch_buffer

> posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
> glob, GLIBC_2_1);
> posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
> glob, GLIBC_2_0);
> posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawn,
> posix_spawn, GLIBC_2_2);
> posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawnp,
> posix_spawnp, GLIBC_2_2);

tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
		   tst-glob_lstat_compat tst-spawn4-compat

And so on.

tests-internal changes -DMODULE_NAME=.  I can't easily do that for
nptl/tst-cleanup4aux.c.

Maybe we could move some of these other tests from tests-internal to
tests because they do not test internals anymore.

Thanks,
Florian
H.J. Lu March 2, 2021, 9:33 p.m. UTC | #7 
On Tue, Mar 2, 2021 at 1:28 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Tue, Mar 2, 2021 at 1:18 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Tue, Mar 2, 2021 at 1:02 PM Florian Weimer <fweimer@redhat.com> wrote:
> >> >
> >> > * H. J. Lu:
> >> >
> >> > > On Tue, Mar 2, 2021 at 12:10 PM Florian Weimer via Libc-alpha
> >> > > <libc-alpha@sourceware.org> wrote:
> >> > >>
> >> > >> This is helpful for testing compat symbols in cases where _ISOMAC
> >> > >> is activated implicitly due to -DMODULE_NAME=testsuite and cannot
> >> > >> be disabled easily.
> >> > >>
> >> > >> ---
> >> > >> v2: Now actually tested in the situation where I need it.
> >> > >>
> >> > >
> >> > > Any particular tests which need this?
> >> >
> >> > nptl/tst-cleanup4aux.c will need this once _pthread_cleanup_push,
> >> > _pthread_cleanup_pop are compatibility symbols.
> >> >
> >>
> >> Do you just want to link against the older version and nothing else?
> >>
> >
> > Why are these tests OK without your patch?
> >
> > malloc/tst-mallocstate.c:compat_symbol_reference (libc,
> > malloc_get_state, malloc_get_state, GLIBC_2_0);
> > malloc/tst-mallocstate.c:compat_symbol_reference (libc,
> > malloc_set_state, malloc_set_state, GLIBC_2_0);
>
> tests-internal := tst-mallocstate tst-scratch_buffer
>
> > posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
> > glob, GLIBC_2_1);
> > posix/tst-glob_lstat_compat.c:compat_symbol_reference (libc, glob,
> > glob, GLIBC_2_0);
> > posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawn,
> > posix_spawn, GLIBC_2_2);
> > posix/tst-spawn4-compat.c:compat_symbol_reference (libc, posix_spawnp,
> > posix_spawnp, GLIBC_2_2);
>
> tests-internal  := bug-regex5 bug-regex20 bug-regex33 \
>                    tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
>                    tst-glob_lstat_compat tst-spawn4-compat
>
> And so on.
>
> tests-internal changes -DMODULE_NAME=.  I can't easily do that for
> nptl/tst-cleanup4aux.c.
>
> Maybe we could move some of these other tests from tests-internal to
> tests because they do not test internals anymore.

Can you submit a patch set to clean up these "internal" tests together
with your patch?

Thanks.
Florian Weimer March 2, 2021, 9:35 p.m. UTC | #8 
* H. J. Lu:

>> tests-internal  := bug-regex5 bug-regex20 bug-regex33 \
>>                    tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
>>                    tst-glob_lstat_compat tst-spawn4-compat
>>
>> And so on.
>>
>> tests-internal changes -DMODULE_NAME=.  I can't easily do that for
>> nptl/tst-cleanup4aux.c.
>>
>> Maybe we could move some of these other tests from tests-internal to
>> tests because they do not test internals anymore.
>
> Can you submit a patch set to clean up these "internal" tests together
> with your patch?

I can try and see if they still build as regular tests.

Thanks,
Florian
Andreas Schwab March 3, 2021, 8:39 a.m. UTC | #9 
On Mär 02 2021, Florian Weimer via Libc-alpha wrote:

> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index ea126ae70c..6e59050da7 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h
> @@ -59,6 +59,19 @@
>  # define IN_MODULE (-1)
>  #endif
>  
> +/* Use __symbol_version_reference to specify the version a symbol
> +   reference should link to.  Use symbol_version or
> +   default_symbol_version for the definition of a versioned symbol.
> +   The difference is that the latter is a no-op in non-shared
> +   builds.  */
> +#ifdef __ASSEMBLER__
> +# define __symbol_version_reference(real, name, version) \
> +     .symver real, name##@##version
> +#else  /* !__ASSEMBLER__ */
> +# define __symbol_version_reference(real, name, version) \
> +  __asm__ (".symver " #real "," #name "@" #version)
> +#endif

Why is the underscore mangling needed?

Andreas.
Florian Weimer March 3, 2021, 8:53 a.m. UTC | #10 
* Andreas Schwab:

> On Mär 02 2021, Florian Weimer via Libc-alpha wrote:
>
>> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
>> index ea126ae70c..6e59050da7 100644
>> --- a/include/libc-symbols.h
>> +++ b/include/libc-symbols.h
>> @@ -59,6 +59,19 @@
>>  # define IN_MODULE (-1)
>>  #endif
>>  
>> +/* Use __symbol_version_reference to specify the version a symbol
>> +   reference should link to.  Use symbol_version or
>> +   default_symbol_version for the definition of a versioned symbol.
>> +   The difference is that the latter is a no-op in non-shared
>> +   builds.  */
>> +#ifdef __ASSEMBLER__
>> +# define __symbol_version_reference(real, name, version) \
>> +     .symver real, name##@##version
>> +#else  /* !__ASSEMBLER__ */
>> +# define __symbol_version_reference(real, name, version) \
>> +  __asm__ (".symver " #real "," #name "@" #version)
>> +#endif
>
> Why is the underscore mangling needed?

I expected many conform test failures because the comment says the file
is implicitly included everywhere.  But apparently that is not the case,
so using a non-mangled name is fine.  A few macros (like IS_IN) are
already unconditionally defined.

Thanks,
Florian
Florian Weimer March 3, 2021, 10:04 a.m. UTC | #11 
* Florian Weimer via Libc-alpha:

> * H. J. Lu:
>
>>> tests-internal  := bug-regex5 bug-regex20 bug-regex33 \
>>>                    tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
>>>                    tst-glob_lstat_compat tst-spawn4-compat
>>>
>>> And so on.
>>>
>>> tests-internal changes -DMODULE_NAME=.  I can't easily do that for
>>> nptl/tst-cleanup4aux.c.
>>>
>>> Maybe we could move some of these other tests from tests-internal to
>>> tests because they do not test internals anymore.
>>
>> Can you submit a patch set to clean up these "internal" tests together
>> with your patch?
>
> I can try and see if they still build as regular tests.

So there are some more cleanups that would make sense.  I don't think we
should use TEST_COMPAT to disable tests, but rather the make macros in
Versions.mk (which came later).  But that probably needs some
discussion, so I want to do these cleanups in a separate patch.

Thanks,
Florian
Florian Weimer March 5, 2021, 1:06 p.m. UTC | #12 
* H. J. Lu:

> Can you submit a patch set to clean up these "internal" tests together
> with your patch?

I've posted the cleanup patches now.

Thanks,
Florian
diff mbox series

Patch

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index ea126ae70c..6e59050da7 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -59,6 +59,19 @@ 
 # define IN_MODULE (-1)
 #endif
 
+/* Use __symbol_version_reference to specify the version a symbol
+   reference should link to.  Use symbol_version or
+   default_symbol_version for the definition of a versioned symbol.
+   The difference is that the latter is a no-op in non-shared
+   builds.  */
+#ifdef __ASSEMBLER__
+# define __symbol_version_reference(real, name, version) \
+     .symver real, name##@##version
+#else  /* !__ASSEMBLER__ */
+# define __symbol_version_reference(real, name, version) \
+  __asm__ (".symver " #real "," #name "@" #version)
+#endif
+
 #ifndef _ISOMAC
 
 /* This is defined for the compilation of all C library code.  features.h
@@ -396,22 +409,9 @@  for linking")
    past the last element in SET.  */
 #define symbol_set_end_p(set, ptr) ((ptr) >= (void *const *) &__stop_##set)
 
-/* Use symbol_version_reference to specify the version a symbol
-   reference should link to.  Use symbol_version or
-   default_symbol_version for the definition of a versioned symbol.
-   The difference is that the latter is a no-op in non-shared
-   builds.  */
-#ifdef __ASSEMBLER__
-# define symbol_version_reference(real, name, version) \
-     .symver real, name##@##version
-#else  /* !__ASSEMBLER__ */
-# define symbol_version_reference(real, name, version) \
-  __asm__ (".symver " #real "," #name "@" #version)
-#endif
-
 #ifdef SHARED
 # define symbol_version(real, name, version) \
-  symbol_version_reference(real, name, version)
+  __symbol_version_reference(real, name, version)
 # define default_symbol_version(real, name, version) \
      _default_symbol_version(real, name, version)
 # ifdef __ASSEMBLER__
diff --git a/include/shlib-compat.h b/include/shlib-compat.h
index 28baef1ea4..e8adef71bf 100644
--- a/include/shlib-compat.h
+++ b/include/shlib-compat.h
@@ -130,7 +130,7 @@ 
 #define compat_symbol_reference_1(lib, local, symbol, version) \
   compat_symbol_reference_2 (local, symbol, VERSION_##lib##_##version)
 #define compat_symbol_reference_2(local, symbol, name) \
-  symbol_version_reference (local, symbol, name)
+  __symbol_version_reference (local, symbol, name)
 
 /* Export the symbol only for shared-library compatibility.  */
 #define libc_sunrpc_symbol(name, aliasname, version) \