public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
@ 2017-08-31  9:52 Florian Weimer
  2017-08-31 15:23 ` Carlos O'Donell
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2017-08-31  9:52 UTC (permalink / raw)
  To: libc-alpha

Since glibc 2.24, __malloc_initialize_hook is a compat symbol.  As a
result, the link editor does not export a definition of
__malloc_initialize_hook from the main program, so that it no longer
interposes the variable definition in libc.so.  Specifying the symbol
version restores the exported symbol.

2017-08-31  Florian Weimer  <fweimer@redhat.com>

	[BZ #22050]
	* malloc/mcheck-init.c (__malloc_initialize_hook): Use
	compat_symbol_reference to access non-default version.

diff --git a/malloc/mcheck-init.c b/malloc/mcheck-init.c
index 6d2492ef7e..4ebfa868ea 100644
--- a/malloc/mcheck-init.c
+++ b/malloc/mcheck-init.c
@@ -20,6 +20,7 @@
 
 #include <malloc.h>
 #include <mcheck.h>
+#include <shlib-compat.h>
 
 static void
 turn_on_mcheck (void)
@@ -28,3 +29,7 @@ turn_on_mcheck (void)
 }
 
 void (*__malloc_initialize_hook) (void) = turn_on_mcheck;
+/* Slight abuse of compat_symbol_reference because the above is not a
+   reference, but actually a definition.  */
+compat_symbol_reference (libc, __malloc_initialize_hook,
+                         __malloc_initialize_hook, GLIBC_2_0);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31  9:52 [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050] Florian Weimer
@ 2017-08-31 15:23 ` Carlos O'Donell
  2017-08-31 15:28   ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Carlos O'Donell @ 2017-08-31 15:23 UTC (permalink / raw)
  To: Florian Weimer, libc-alpha

On 08/31/2017 04:52 AM, Florian Weimer wrote:
> Since glibc 2.24, __malloc_initialize_hook is a compat symbol.  As a
> result, the link editor does not export a definition of
> __malloc_initialize_hook from the main program, so that it no longer
> interposes the variable definition in libc.so.  Specifying the symbol
> version restores the exported symbol.
> 
> 2017-08-31  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22050]
> 	* malloc/mcheck-init.c (__malloc_initialize_hook): Use
> 	compat_symbol_reference to access non-default version.
>
> diff --git a/malloc/mcheck-init.c b/malloc/mcheck-init.c
> index 6d2492ef7e..4ebfa868ea 100644
> --- a/malloc/mcheck-init.c
> +++ b/malloc/mcheck-init.c
> @@ -20,6 +20,7 @@
>  
>  #include <malloc.h>
>  #include <mcheck.h>
> +#include <shlib-compat.h>
>  
>  static void
>  turn_on_mcheck (void)
> @@ -28,3 +29,7 @@ turn_on_mcheck (void)
>  }
>  
>  void (*__malloc_initialize_hook) (void) = turn_on_mcheck;
> +/* Slight abuse of compat_symbol_reference because the above is not a
> +   reference, but actually a definition.  */
> +compat_symbol_reference (libc, __malloc_initialize_hook,
> +                         __malloc_initialize_hook, GLIBC_2_0);
> 

We deprecated __malloc_initialize_hook in libc.so.6. There is only a compat
symbol there, which can only be interposed by a definition of the same version.

In libmcheck.a we have __malloc_initialize_hook (unversioned) which no longer
interposes the symbol in libc.so.6 (it would have interposed the default if
there was one).

So what does the compat_symbol_reference create in the libmcheck.a? An undefined
reference to __malloc_initialize_hook@GLIBC_2_0?

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 15:23 ` Carlos O'Donell
@ 2017-08-31 15:28   ` Florian Weimer
  2017-08-31 15:37     ` Carlos O'Donell
  2017-08-31 16:03     ` Joseph Myers
  0 siblings, 2 replies; 16+ messages in thread
From: Florian Weimer @ 2017-08-31 15:28 UTC (permalink / raw)
  To: Carlos O'Donell, libc-alpha

On 08/31/2017 05:23 PM, Carlos O'Donell wrote:

> We deprecated __malloc_initialize_hook in libc.so.6. There is only a compat
> symbol there, which can only be interposed by a definition of the same version.
> 
> In libmcheck.a we have __malloc_initialize_hook (unversioned) which no longer
> interposes the symbol in libc.so.6 (it would have interposed the default if
> there was one).
> 
> So what does the compat_symbol_reference create in the libmcheck.a? An undefined
> reference to __malloc_initialize_hook@GLIBC_2_0?

Interposition happens only if __malloc_initialize_hook is listed in the
.dynsym section of the executable.  At least some versions of binutils
will not add the symbol to the .dynsym section if it is unversioned and
there is a definition in a DSO which lacks a default version.  Arguably
this is a bug in the link editor.

The existing libmcheck.a works if you link the main program with
--export-dynamic or otherwise arrange for an exported
__malloc_initialize_hook (even unversioned).

What happens with my patch is that the definition has the proper symbol
version, so the link editor matches it against the definition in libc.so
and notices that it needs to be listed in .dynsym.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 15:28   ` Florian Weimer
@ 2017-08-31 15:37     ` Carlos O'Donell
  2017-08-31 15:42       ` Zack Weinberg
  2017-08-31 16:03     ` Joseph Myers
  1 sibling, 1 reply; 16+ messages in thread
From: Carlos O'Donell @ 2017-08-31 15:37 UTC (permalink / raw)
  To: Florian Weimer, libc-alpha

On 08/31/2017 10:28 AM, Florian Weimer wrote:
> On 08/31/2017 05:23 PM, Carlos O'Donell wrote:
> 
>> We deprecated __malloc_initialize_hook in libc.so.6. There is only a compat
>> symbol there, which can only be interposed by a definition of the same version.
>>
>> In libmcheck.a we have __malloc_initialize_hook (unversioned) which no longer
>> interposes the symbol in libc.so.6 (it would have interposed the default if
>> there was one).
>>
>> So what does the compat_symbol_reference create in the libmcheck.a? An undefined
>> reference to __malloc_initialize_hook@GLIBC_2_0?
> 
> Interposition happens only if __malloc_initialize_hook is listed in the
> .dynsym section of the executable.  At least some versions of binutils
> will not add the symbol to the .dynsym section if it is unversioned and
> there is a definition in a DSO which lacks a default version.  Arguably
> this is a bug in the link editor.
> 
> The existing libmcheck.a works if you link the main program with
> --export-dynamic or otherwise arrange for an exported
> __malloc_initialize_hook (even unversioned).
> 
> What happens with my patch is that the definition has the proper symbol
> version, so the link editor matches it against the definition in libc.so
> and notices that it needs to be listed in .dynsym.

So it makes it look like there was an old reference to
__malloc_initialize_hook@GLIBC_2.0?

In that case I think we should not play games with the existing macro.

I would like to see a new macro that does what it says, rather than use the
existing macro in the wrong way. Even if the new macro is just a copy.

This looks like a real problem for glibc, particularly if we need to continue
to use, at least internally, certain old versions of symbols. So having a
new macro for this is fine.

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 15:37     ` Carlos O'Donell
@ 2017-08-31 15:42       ` Zack Weinberg
  2017-08-31 16:00         ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Zack Weinberg @ 2017-08-31 15:42 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library

On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> I would like to see a new macro that does what it says, rather than use the
> existing macro in the wrong way. Even if the new macro is just a copy.
>
> This looks like a real problem for glibc, particularly if we need to continue
> to use, at least internally, certain old versions of symbols. So having a
> new macro for this is fine.

I see immediate uses for this macro in the test suite, verifying that
compat symbols continue to work correctly...  (particularly thinking
of the messy and totally untested old-FILE support).

zw

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 15:42       ` Zack Weinberg
@ 2017-08-31 16:00         ` Florian Weimer
  2017-08-31 16:04           ` Joseph Myers
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2017-08-31 16:00 UTC (permalink / raw)
  To: Zack Weinberg, Carlos O'Donell; +Cc: GNU C Library

On 08/31/2017 05:42 PM, Zack Weinberg wrote:
> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> I would like to see a new macro that does what it says, rather than use the
>> existing macro in the wrong way. Even if the new macro is just a copy.
>>
>> This looks like a real problem for glibc, particularly if we need to continue
>> to use, at least internally, certain old versions of symbols. So having a
>> new macro for this is fine.
> 
> I see immediate uses for this macro in the test suite, verifying that
> compat symbols continue to work correctly...  (particularly thinking
> of the messy and totally untested old-FILE support).

That's the exact purpose of compat_symbol_reference.  I think Carlos is
objecting to its use for a *definition*.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 15:28   ` Florian Weimer
  2017-08-31 15:37     ` Carlos O'Donell
@ 2017-08-31 16:03     ` Joseph Myers
  2017-08-31 16:34       ` Florian Weimer
  1 sibling, 1 reply; 16+ messages in thread
From: Joseph Myers @ 2017-08-31 16:03 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Carlos O'Donell, libc-alpha

On Thu, 31 Aug 2017, Florian Weimer wrote:

> Interposition happens only if __malloc_initialize_hook is listed in the
> .dynsym section of the executable.  At least some versions of binutils
> will not add the symbol to the .dynsym section if it is unversioned and
> there is a definition in a DSO which lacks a default version.  Arguably
> this is a bug in the link editor.

Sounds like a feature to me, not a bug.  After all, it's essentially what 
makes it possible for us to obsolete (stop working for new programs as far 
as possible) symbols whose interface is that the program interposes them, 
such as matherr.  Effectively, a DSO definition that lacks a default 
version should be treated exactly the same at static link time as no DSO 
definition at all.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:00         ` Florian Weimer
@ 2017-08-31 16:04           ` Joseph Myers
  2017-08-31 16:08             ` Carlos O'Donell
  0 siblings, 1 reply; 16+ messages in thread
From: Joseph Myers @ 2017-08-31 16:04 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Zack Weinberg, Carlos O'Donell, GNU C Library

On Thu, 31 Aug 2017, Florian Weimer wrote:

> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
> > On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> >> I would like to see a new macro that does what it says, rather than use the
> >> existing macro in the wrong way. Even if the new macro is just a copy.
> >>
> >> This looks like a real problem for glibc, particularly if we need to continue
> >> to use, at least internally, certain old versions of symbols. So having a
> >> new macro for this is fine.
> > 
> > I see immediate uses for this macro in the test suite, verifying that
> > compat symbols continue to work correctly...  (particularly thinking
> > of the messy and totally untested old-FILE support).
> 
> That's the exact purpose of compat_symbol_reference.  I think Carlos is
> objecting to its use for a *definition*.

Well, I used it for the definitions of matherr and _LIB_VERSION in my 
tests of those compat symbols, because it does exactly what's expected: 
makes the definitions in the tests refer to the same entity as the compat 
symbols in the shared libraries, rather than being completely independent 
entities as they would by default.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:04           ` Joseph Myers
@ 2017-08-31 16:08             ` Carlos O'Donell
  2017-08-31 16:18               ` Joseph Myers
  2017-09-18 14:24               ` Florian Weimer
  0 siblings, 2 replies; 16+ messages in thread
From: Carlos O'Donell @ 2017-08-31 16:08 UTC (permalink / raw)
  To: Joseph Myers, Florian Weimer; +Cc: Zack Weinberg, GNU C Library

On 08/31/2017 11:04 AM, Joseph Myers wrote:
> On Thu, 31 Aug 2017, Florian Weimer wrote:
> 
>> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
>>> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>> I would like to see a new macro that does what it says, rather than use the
>>>> existing macro in the wrong way. Even if the new macro is just a copy.
>>>>
>>>> This looks like a real problem for glibc, particularly if we need to continue
>>>> to use, at least internally, certain old versions of symbols. So having a
>>>> new macro for this is fine.
>>>
>>> I see immediate uses for this macro in the test suite, verifying that
>>> compat symbols continue to work correctly...  (particularly thinking
>>> of the messy and totally untested old-FILE support).
>>
>> That's the exact purpose of compat_symbol_reference.  I think Carlos is
>> objecting to its use for a *definition*.
> 
> Well, I used it for the definitions of matherr and _LIB_VERSION in my 
> tests of those compat symbols, because it does exactly what's expected: 
> makes the definitions in the tests refer to the same entity as the compat 
> symbols in the shared libraries, rather than being completely independent 
> entities as they would by default.
 
While it does what's expected, the macro API wasn't designed with that in
mind was it? I am objecting to using a macro not designed for this
purpose, and suggesting a new macro that makes the intent clear.

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:08             ` Carlos O'Donell
@ 2017-08-31 16:18               ` Joseph Myers
  2017-09-18 14:24               ` Florian Weimer
  1 sibling, 0 replies; 16+ messages in thread
From: Joseph Myers @ 2017-08-31 16:18 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: Florian Weimer, Zack Weinberg, GNU C Library

On Thu, 31 Aug 2017, Carlos O'Donell wrote:

> >> That's the exact purpose of compat_symbol_reference.  I think Carlos is
> >> objecting to its use for a *definition*.
> > 
> > Well, I used it for the definitions of matherr and _LIB_VERSION in my 
> > tests of those compat symbols, because it does exactly what's expected: 
> > makes the definitions in the tests refer to the same entity as the compat 
> > symbols in the shared libraries, rather than being completely independent 
> > entities as they would by default.
>  
> While it does what's expected, the macro API wasn't designed with that in
> mind was it? I am objecting to using a macro not designed for this
> purpose, and suggesting a new macro that makes the intent clear.

I think "reference" naturally covers both cases of making the symbol refer 
to the same entity in both places.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:03     ` Joseph Myers
@ 2017-08-31 16:34       ` Florian Weimer
  2017-08-31 16:45         ` Joseph Myers
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2017-08-31 16:34 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Carlos O'Donell, libc-alpha

On 08/31/2017 06:02 PM, Joseph Myers wrote:
> On Thu, 31 Aug 2017, Florian Weimer wrote:
> 
>> Interposition happens only if __malloc_initialize_hook is listed in the
>> .dynsym section of the executable.  At least some versions of binutils
>> will not add the symbol to the .dynsym section if it is unversioned and
>> there is a definition in a DSO which lacks a default version.  Arguably
>> this is a bug in the link editor.
> 
> Sounds like a feature to me, not a bug.  After all, it's essentially what 
> makes it possible for us to obsolete (stop working for new programs as far 
> as possible) symbols whose interface is that the program interposes them, 
> such as matherr.  Effectively, a DSO definition that lacks a default 
> version should be treated exactly the same at static link time as no DSO 
> definition at all.

But that's not what happens if you compile with --export-dynamic.  That
--export-dynamic changes behavior here is very surprising.  That's why I
think this could be considered a bug in the link editor.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:34       ` Florian Weimer
@ 2017-08-31 16:45         ` Joseph Myers
  0 siblings, 0 replies; 16+ messages in thread
From: Joseph Myers @ 2017-08-31 16:45 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Carlos O'Donell, libc-alpha

On Thu, 31 Aug 2017, Florian Weimer wrote:

> On 08/31/2017 06:02 PM, Joseph Myers wrote:
> > On Thu, 31 Aug 2017, Florian Weimer wrote:
> > 
> >> Interposition happens only if __malloc_initialize_hook is listed in the
> >> .dynsym section of the executable.  At least some versions of binutils
> >> will not add the symbol to the .dynsym section if it is unversioned and
> >> there is a definition in a DSO which lacks a default version.  Arguably
> >> this is a bug in the link editor.
> > 
> > Sounds like a feature to me, not a bug.  After all, it's essentially what 
> > makes it possible for us to obsolete (stop working for new programs as far 
> > as possible) symbols whose interface is that the program interposes them, 
> > such as matherr.  Effectively, a DSO definition that lacks a default 
> > version should be treated exactly the same at static link time as no DSO 
> > definition at all.
> 
> But that's not what happens if you compile with --export-dynamic.  That
> --export-dynamic changes behavior here is very surprising.  That's why I
> think this could be considered a bug in the link editor.

That --export-dynamic changes things sounds like a consequence of the rule 
that an unversioned symbol can be linked to the base symbol version, 
because symbol versioning support wasn't added until glibc 2.1 and 
binaries with unversioned references to glibc 2.0 symbols needed to be 
supported.  I'm not sure how to fix that (stop --export-dynamic allowing 
symbols to link to compat symbols) - maybe that special rule should be 
disabled for glibc symbols if a program has any dependence on a glibc 
version greater than 2.0, is there a way we can make sure all newly linked 
programs have such dependence to stop that rule applying to them?  (If 
--export-dynamic results in *versioned* exports from the executable for 
such symbols as matherr or __malloc_initialize_hook, that sounds like a 
bug in ld.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-08-31 16:08             ` Carlos O'Donell
  2017-08-31 16:18               ` Joseph Myers
@ 2017-09-18 14:24               ` Florian Weimer
  2017-10-05 10:46                 ` Florian Weimer
  1 sibling, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2017-09-18 14:24 UTC (permalink / raw)
  To: Carlos O'Donell, Joseph Myers; +Cc: Zack Weinberg, GNU C Library

On 08/31/2017 06:08 PM, Carlos O'Donell wrote:
> On 08/31/2017 11:04 AM, Joseph Myers wrote:
>> On Thu, 31 Aug 2017, Florian Weimer wrote:
>>
>>> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
>>>> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>> I would like to see a new macro that does what it says, rather than use the
>>>>> existing macro in the wrong way. Even if the new macro is just a copy.
>>>>>
>>>>> This looks like a real problem for glibc, particularly if we need to continue
>>>>> to use, at least internally, certain old versions of symbols. So having a
>>>>> new macro for this is fine.
>>>>
>>>> I see immediate uses for this macro in the test suite, verifying that
>>>> compat symbols continue to work correctly...  (particularly thinking
>>>> of the messy and totally untested old-FILE support).
>>>
>>> That's the exact purpose of compat_symbol_reference.  I think Carlos is
>>> objecting to its use for a *definition*.
>>
>> Well, I used it for the definitions of matherr and _LIB_VERSION in my
>> tests of those compat symbols, because it does exactly what's expected:
>> makes the definitions in the tests refer to the same entity as the compat
>> symbols in the shared libraries, rather than being completely independent
>> entities as they would by default.
>   
> While it does what's expected, the macro API wasn't designed with that in
> mind was it?

I would have used it in tst-mallocstate for __malloc_initialize_hook if 
I had understood what was going on.  I think the usages are so similar 
that we do not need a new macro.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-09-18 14:24               ` Florian Weimer
@ 2017-10-05 10:46                 ` Florian Weimer
  2017-10-11 17:00                   ` Carlos O'Donell
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2017-10-05 10:46 UTC (permalink / raw)
  To: Carlos O'Donell, Joseph Myers; +Cc: Zack Weinberg, GNU C Library

On 09/18/2017 04:24 PM, Florian Weimer wrote:
> On 08/31/2017 06:08 PM, Carlos O'Donell wrote:
>> On 08/31/2017 11:04 AM, Joseph Myers wrote:
>>> On Thu, 31 Aug 2017, Florian Weimer wrote:
>>>
>>>> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell 
>>>>> <carlos@redhat.com> wrote:
>>>>>> I would like to see a new macro that does what it says, rather 
>>>>>> than use the
>>>>>> existing macro in the wrong way. Even if the new macro is just a 
>>>>>> copy.
>>>>>>
>>>>>> This looks like a real problem for glibc, particularly if we need 
>>>>>> to continue
>>>>>> to use, at least internally, certain old versions of symbols. So 
>>>>>> having a
>>>>>> new macro for this is fine.
>>>>>
>>>>> I see immediate uses for this macro in the test suite, verifying that
>>>>> compat symbols continue to work correctly...  (particularly thinking
>>>>> of the messy and totally untested old-FILE support).
>>>>
>>>> That's the exact purpose of compat_symbol_reference.  I think Carlos is
>>>> objecting to its use for a *definition*.
>>>
>>> Well, I used it for the definitions of matherr and _LIB_VERSION in my
>>> tests of those compat symbols, because it does exactly what's expected:
>>> makes the definitions in the tests refer to the same entity as the 
>>> compat
>>> symbols in the shared libraries, rather than being completely 
>>> independent
>>> entities as they would by default.
>> While it does what's expected, the macro API wasn't designed with that in
>> mind was it?
> 
> I would have used it in tst-mallocstate for __malloc_initialize_hook if 
> I had understood what was going on.  I think the usages are so similar 
> that we do not need a new macro.

Carlos, do you still have objections to the patch as posted?

   <https://sourceware.org/ml/libc-alpha/2017-08/msg01317.html>

I do not think we need another macro with a different name for this.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-10-05 10:46                 ` Florian Weimer
@ 2017-10-11 17:00                   ` Carlos O'Donell
  2017-10-16 18:45                     ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Carlos O'Donell @ 2017-10-11 17:00 UTC (permalink / raw)
  To: Florian Weimer, Joseph Myers; +Cc: Zack Weinberg, GNU C Library

On 10/05/2017 03:45 AM, Florian Weimer wrote:
> On 09/18/2017 04:24 PM, Florian Weimer wrote:
>> On 08/31/2017 06:08 PM, Carlos O'Donell wrote:
>>> On 08/31/2017 11:04 AM, Joseph Myers wrote:
>>>> On Thu, 31 Aug 2017, Florian Weimer wrote:
>>>>
>>>>> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
>>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>>> I would like to see a new macro that does what it says, rather than use the
>>>>>>> existing macro in the wrong way. Even if the new macro is just a copy.
>>>>>>>
>>>>>>> This looks like a real problem for glibc, particularly if we need to continue
>>>>>>> to use, at least internally, certain old versions of symbols. So having a
>>>>>>> new macro for this is fine.
>>>>>>
>>>>>> I see immediate uses for this macro in the test suite, verifying that
>>>>>> compat symbols continue to work correctly...  (particularly thinking
>>>>>> of the messy and totally untested old-FILE support).
>>>>>
>>>>> That's the exact purpose of compat_symbol_reference.  I think Carlos is
>>>>> objecting to its use for a *definition*.
>>>>
>>>> Well, I used it for the definitions of matherr and _LIB_VERSION in my
>>>> tests of those compat symbols, because it does exactly what's expected:
>>>> makes the definitions in the tests refer to the same entity as the compat
>>>> symbols in the shared libraries, rather than being completely independent
>>>> entities as they would by default.
>>> While it does what's expected, the macro API wasn't designed with that in
>>> mind was it?
>>
>> I would have used it in tst-mallocstate for
>> __malloc_initialize_hook if I had understood what was going on.  I
>> think the usages are so similar that we do not need a new macro.
> 
> Carlos, do you still have objections to the patch as posted?
> 
>   <https://sourceware.org/ml/libc-alpha/2017-08/msg01317.html>
> 
> I do not think we need another macro with a different name for this.

I would like to see something like this added:

diff --git a/include/shlib-compat.h b/include/shlib-compat.h
index d872afc..ba99f9b 100644
--- a/include/shlib-compat.h
+++ b/include/shlib-compat.h
@@ -78,8 +78,12 @@
 
 #endif
 
-/* Use compat_symbol_reference for a reference to a specific version
-   of a symbol.  Use compat_symbol to define such a symbol.  */
+/* Use compat_symbol_reference for a reference *or* definition of a 
+   specific version of a symbol.  Definitions are primarily used to
+   ensure tests reference the exact compat symbol required, or define an
+   interposing symbol of the right version e.g. __malloc_initialize_hook
+   in mcheck-init.c.  Use compat_symbol to define such a symbol within
+   the shared libraries that are built for users.  */
 #define compat_symbol_reference(lib, local, symbol, version) \
   compat_symbol_reference_1 (lib, local, symbol, version)
 #define compat_symbol_reference_1(lib, local, symbol, version) \
---

And remove the "this is a hack" comments from your patch, since now you
are using the macro API within the usages of the definition.

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]
  2017-10-11 17:00                   ` Carlos O'Donell
@ 2017-10-16 18:45                     ` Florian Weimer
  0 siblings, 0 replies; 16+ messages in thread
From: Florian Weimer @ 2017-10-16 18:45 UTC (permalink / raw)
  To: Carlos O'Donell, Joseph Myers; +Cc: Zack Weinberg, GNU C Library

On 10/11/2017 07:00 PM, Carlos O'Donell wrote:
> On 10/05/2017 03:45 AM, Florian Weimer wrote:
>> On 09/18/2017 04:24 PM, Florian Weimer wrote:
>>> On 08/31/2017 06:08 PM, Carlos O'Donell wrote:
>>>> On 08/31/2017 11:04 AM, Joseph Myers wrote:
>>>>> On Thu, 31 Aug 2017, Florian Weimer wrote:
>>>>>
>>>>>> On 08/31/2017 05:42 PM, Zack Weinberg wrote:
>>>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>>>> I would like to see a new macro that does what it says, rather than use the
>>>>>>>> existing macro in the wrong way. Even if the new macro is just a copy.
>>>>>>>>
>>>>>>>> This looks like a real problem for glibc, particularly if we need to continue
>>>>>>>> to use, at least internally, certain old versions of symbols. So having a
>>>>>>>> new macro for this is fine.
>>>>>>>
>>>>>>> I see immediate uses for this macro in the test suite, verifying that
>>>>>>> compat symbols continue to work correctly...  (particularly thinking
>>>>>>> of the messy and totally untested old-FILE support).
>>>>>>
>>>>>> That's the exact purpose of compat_symbol_reference.  I think Carlos is
>>>>>> objecting to its use for a *definition*.
>>>>>
>>>>> Well, I used it for the definitions of matherr and _LIB_VERSION in my
>>>>> tests of those compat symbols, because it does exactly what's expected:
>>>>> makes the definitions in the tests refer to the same entity as the compat
>>>>> symbols in the shared libraries, rather than being completely independent
>>>>> entities as they would by default.
>>>> While it does what's expected, the macro API wasn't designed with that in
>>>> mind was it?
>>>
>>> I would have used it in tst-mallocstate for
>>> __malloc_initialize_hook if I had understood what was going on.  I
>>> think the usages are so similar that we do not need a new macro.
>>
>> Carlos, do you still have objections to the patch as posted?
>>
>>    <https://sourceware.org/ml/libc-alpha/2017-08/msg01317.html>
>>
>> I do not think we need another macro with a different name for this.
> 
> I would like to see something like this added:
> 
> diff --git a/include/shlib-compat.h b/include/shlib-compat.h
> index d872afc..ba99f9b 100644
> --- a/include/shlib-compat.h
> +++ b/include/shlib-compat.h
> @@ -78,8 +78,12 @@
>   
>   #endif
>   
> -/* Use compat_symbol_reference for a reference to a specific version
> -   of a symbol.  Use compat_symbol to define such a symbol.  */
> +/* Use compat_symbol_reference for a reference *or* definition of a
> +   specific version of a symbol.  Definitions are primarily used to
> +   ensure tests reference the exact compat symbol required, or define an
> +   interposing symbol of the right version e.g. __malloc_initialize_hook
> +   in mcheck-init.c.  Use compat_symbol to define such a symbol within
> +   the shared libraries that are built for users.  */
>   #define compat_symbol_reference(lib, local, symbol, version) \
>     compat_symbol_reference_1 (lib, local, symbol, version)
>   #define compat_symbol_reference_1(lib, local, symbol, version) \
> ---
> 
> And remove the "this is a hack" comments from your patch, since now you
> are using the macro API within the usages of the definition.

Fine, I'm going to push this change (with trailing whitespace removed).

Thanks,
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-10-16 18:45 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31  9:52 [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050] Florian Weimer
2017-08-31 15:23 ` Carlos O'Donell
2017-08-31 15:28   ` Florian Weimer
2017-08-31 15:37     ` Carlos O'Donell
2017-08-31 15:42       ` Zack Weinberg
2017-08-31 16:00         ` Florian Weimer
2017-08-31 16:04           ` Joseph Myers
2017-08-31 16:08             ` Carlos O'Donell
2017-08-31 16:18               ` Joseph Myers
2017-09-18 14:24               ` Florian Weimer
2017-10-05 10:46                 ` Florian Weimer
2017-10-11 17:00                   ` Carlos O'Donell
2017-10-16 18:45                     ` Florian Weimer
2017-08-31 16:03     ` Joseph Myers
2017-08-31 16:34       ` Florian Weimer
2017-08-31 16:45         ` Joseph Myers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).