This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3] Extra dlopen/getpagesize static executable tests
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: <libc-alpha at sourceware dot org>
- Date: Fri, 28 Jun 2013 17:51:25 +0100
- Subject: Re: [PATCH v3] Extra dlopen/getpagesize static executable tests
- References: <alpine dot DEB dot 1 dot 10 dot 1301152056590 dot 4834 at tp dot orcam dot me dot uk> <20130116215545 dot 7A37A2C0B0 at topped-with-meat dot com> <alpine dot DEB dot 1 dot 10 dot 1301240655220 dot 4834 at tp dot orcam dot me dot uk> <20130531200059 dot C94C02C077 at topped-with-meat dot com> <alpine dot DEB dot 1 dot 10 dot 1306140202520 dot 16287 at tp dot orcam dot me dot uk> <20130620000549 dot 416D12C111 at topped-with-meat dot com> <alpine dot DEB dot 1 dot 10 dot 1306271748120 dot 16287 at tp dot orcam dot me dot uk> <20130627214307 dot 94C7F2C080 at topped-with-meat dot com>
On Thu, 27 Jun 2013, Roland McGrath wrote:
> > Mapping with RTLD_GLOBAL relies on the fix to be in place.
>
> If this intended to be a test for RTLD_GLOBAL crashing, then it should at
> least have a comment saying that's the reason to use the flag. But IMHO
> using RTLD_GLOBAL in a test should be confined to tests that actually
> verify the RTLD_GLOBAL functionality in some fashion.
>
> > > I don't understand why this is using RTLD_GLOBAL.
> > > That does not seem to be material to this test.
> >
> > This is an artefact from the original use of this test. I've switched it
> > to RTLD_LOCAL now, though frankly I don't think any choice made between
> > the two really matters for the scope of the test. The use of RTLD_LOCAL
> > avoids BZ #15022 however.
>
> Use of RTLD_GLOBAL is rare and strange. Nobody should ever use it without
> a clear reason. Using it always expands the scope of the test to include
> the RTLD_GLOBAL implementation.
Understood. Thanks for your insights.
> > I have adjusted the comments and mentally noted the special case (hoping
> > that it sticks). And after a bit of thinking I have concluded tststatic5
> > doesn't really bring anything new here, so I have discarded it altogether.
>
> You say this, but this patch adds tststatic5. So I don't know what you mean.
Off-by-one error, sorry about this. s/tststatic5/tststatic6/ here...
> > Given that tststatic4 does not trip on BZ #15022 anymore, this change no
> > longer has a reason to refer to it.
>
> That makes sense.
... and s/tststatic4/tststatic5/ here.
> > OK to apply?
>
> Looks OK to me.
I have committed it now, thanks for your review.
Maciej