Message ID | 60d34595-b3a7-c170-d325-59b1234e3479@redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 8902 invoked by alias); 11 Oct 2017 17:00:39 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 7049 invoked by uid 89); 11 Oct 2017 17:00:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f173.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=ZA8w3pNmsPcSYivwvNmyr9A2idmVQ9/cptX65h6sw2w=; b=n06MAk46o26YhAP5+ImDyPgI3BkPxPc5s6n/DUeHedyc5o5VDG71xw38fXLb11a9V7 HEt6bfB99MUBKTwvBbFFltqePCL1foK/iYZThyxRbVEDSILNiowixIHVZN9Kf0odSftO 1aCHfhz59HZYsTQ7+YIPriYmr3UsZ8tIFUX1eShLMSpLBsJ1idyiSQZCRjlbICls3gLo lc11ZQKeHDWOnXTb/URYHKEM+i73TkUQy114ThhSxWfkbH7f1oKR3wEQp+tyvRsOD5dV Xlm5Rj4InoY4Bt2RZJKhyuDvMOX2qnnSwL+O4e/bOES/tSZipJzz5RnViaxJ8DRmvht3 ujtA== X-Gm-Message-State: AMCzsaV/fAJNdyNPlE6hzqET/hJ6fQWQDSceOvyVgNyGfVFc/Y8pJOhu RWmtW7n2AQhobZnquezH57JTEuDD61k= X-Google-Smtp-Source: AOwi7QC8vhhCeCOU1QEGNa94WRjyXxJcUxOmE4/h/0udBOUxYOOWc9pSHUBpqm6GEurLwwKIqGV4fw== X-Received: by 10.200.27.171 with SMTP id z40mr467560qtj.151.1507741232004; Wed, 11 Oct 2017 10:00:32 -0700 (PDT) Subject: Re: [PATCH] malloc: Use compat_symbol_reference in libmcheck [BZ #22050] To: Florian Weimer <fweimer@redhat.com>, Joseph Myers <joseph@codesourcery.com> Cc: Zack Weinberg <zackw@panix.com>, GNU C Library <libc-alpha@sourceware.org> References: <20170831095213.42A4F43994318@oldenburg.str.redhat.com> <2269f61e-d718-b90e-6d08-4e325dad4ac9@redhat.com> <c16af5a5-5106-cab5-98d3-210b23d912bb@redhat.com> <2ad943f4-1b66-5df7-b348-ab0652716470@redhat.com> <CAKCAbMg1iZxyWjAwMvTKYg3dyssn2D1YdSQcLnaaGko8bkNFhg@mail.gmail.com> <61a94166-1768-4014-27f7-34a9a6ff24b8@redhat.com> <alpine.DEB.2.20.1708311603280.24662@digraph.polyomino.org.uk> <b619bf25-af58-5435-b59f-95900443de67@redhat.com> <5bf3d95b-cdfb-c55f-478f-be61baa846d3@redhat.com> <834aa29d-d8ad-b516-ab12-98b99ad4faf5@redhat.com> From: Carlos O'Donell <carlos@redhat.com> Message-ID: <60d34595-b3a7-c170-d325-59b1234e3479@redhat.com> Date: Wed, 11 Oct 2017 10:00:27 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <834aa29d-d8ad-b516-ab12-98b99ad4faf5@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit |
Commit Message
Carlos O'Donell
Oct. 11, 2017, 5 p.m. UTC
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.
Comments
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
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) \