Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050]

[Carlos O'Donell <carlos at redhat dot com>]

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.