This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]