malloc: Use compat_symbol_reference in libmcheck [BZ #22050]

  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:

---

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.
  

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