Remove support for "rtld_" prefix on solib-svr4 probes (was: Re: [RFA] Fix PR gdb/17016: Expect for probe "map_complete" instead of "rtld_map_complete")
Commit Message
Thanks for the review.
On Thursday, September 25 2014, Pedro Alves wrote:
> Well, AFAICS, upstream GDB still supports F17's probes. See
> svr4_create_solib_event_breakpoints:
>
> memset (probes, 0, sizeof (probes));
> for (i = 0; i < NUM_PROBES; i++)
> {
> const char *name = probe_info[i].name;
> struct probe *p;
> char buf[32];
>
> /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
> shipped with an early version of the probes code in
> which the probes' names were prefixed with "rtld_"
> and the "map_failed" probe did not exist. The
> locations of the probes are otherwise the same, so
> we check for probes with prefixed names if probes
> with unprefixed names are not present. */
> if (with_prefix)
> {
> xsnprintf (buf, sizeof (buf), "rtld_%s", name);
> name = buf;
> }
>
> probes[i]
Indeed it does, thanks for catching this.
> So it seems to me the test should cope with both variants.
Or maybe we should simplify this code and remove this support.
Really, Fedora 17 was EOL'ed more than 1 year ago:
<https://lists.fedoraproject.org/pipermail/announce/2013-July/003177.html>
And we are already on Fedora 20, moving towards Fedora 21. Also, this
code was needed because a patch present in Fedora 17's glibc, so I think
it is fair to leave this to be handled by Fedora GDB if needed (but it
won't be, because the upstream glibc patches already made into Fedora
too).
I am sending a patch to remove the support, tell me what you think. I'm
in the "let's clean GDB code" mode, in order to avoid having to handle
with dead code all around...
> But, on a cursory look, I can't see what is being tested by this
> test that wouldn't work without probes? If the test would pass just
> the same without probes, I think we should just remove the
> probe probing. OTOH, if this is testing functionally that only
> works if probes are available, then I still think the test is
> lacking a comment.
>
> I also find it a bit odd to check for a probe point that GDB
> doesn't seem to be using:
>
> static const struct probe_info probe_info[] =
> {
> { "init_start", DO_NOTHING },
> { "init_complete", FULL_RELOAD },
> { "map_start", DO_NOTHING },
> { "map_failed", DO_NOTHING },
> { "reloc_complete", UPDATE_OR_RELOAD },
> { "unmap_start", DO_NOTHING },
> { "unmap_complete", FULL_RELOAD },
> };
I will leave these comments to Gary, because he wrote the code. But I
can look at it if needed, of course.
So, what do you think of this patch?
Comments
On 09/25/2014 09:47 PM, Sergio Durigan Junior wrote:
> Thanks for the review.
>
> On Thursday, September 25 2014, Pedro Alves wrote:
>
>> Well, AFAICS, upstream GDB still supports F17's probes. See
>> svr4_create_solib_event_breakpoints:
>>
>> memset (probes, 0, sizeof (probes));
>> for (i = 0; i < NUM_PROBES; i++)
>> {
>> const char *name = probe_info[i].name;
>> struct probe *p;
>> char buf[32];
>>
>> /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
>> shipped with an early version of the probes code in
>> which the probes' names were prefixed with "rtld_"
>> and the "map_failed" probe did not exist. The
>> locations of the probes are otherwise the same, so
>> we check for probes with prefixed names if probes
>> with unprefixed names are not present. */
>> if (with_prefix)
>> {
>> xsnprintf (buf, sizeof (buf), "rtld_%s", name);
>> name = buf;
>> }
>>
>> probes[i]
>
> Indeed it does, thanks for catching this.
>
>> So it seems to me the test should cope with both variants.
>
> Or maybe we should simplify this code and remove this support.
>
> Really, Fedora 17 was EOL'ed more than 1 year ago:
>
> <https://lists.fedoraproject.org/pipermail/announce/2013-July/003177.html>
>
> And we are already on Fedora 20, moving towards Fedora 21. Also, this
> code was needed because a patch present in Fedora 17's glibc, so I think
> it is fair to leave this to be handled by Fedora GDB if needed (but it
> won't be, because the upstream glibc patches already made into Fedora
> too).
There's RHEL (at least, per the comment) 6.4 too, which isn't EOL'ed,
though. It's reasonable to expect that people may still want to
build/test upstream gdb on those?
Thanks,
Pedro Alves
On Thursday, September 25 2014, Pedro Alves wrote:
> There's RHEL (at least, per the comment) 6.4 too, which isn't EOL'ed,
> though. It's reasonable to expect that people may still want to
> build/test upstream gdb on those?
Damn, I forgot to talk about RHEL.
But yeah, RHEL-6.x is not EOL'ed, but the GDB that comes with it is from
Red Hat as well, and contains all the necessary patches to deal with
whatever "internal idiosyncrasy" that it may need. OTOH, I know Gary
has been using RHEL-6.5 to test his upstream patches, so to answer your
question, it is still possible that people may still want to buid/test
upstream GDB there.
My opinion is that we shouldn't need to worry about internals of each
distro (I know we *have to do that* sometimes, but that's not an
excuse), so I still hold the cleanup patch for approval :-).
On 09/25/2014 10:23 PM, Sergio Durigan Junior wrote:
> On Thursday, September 25 2014, Pedro Alves wrote:
>
>> There's RHEL (at least, per the comment) 6.4 too, which isn't EOL'ed,
>> though. It's reasonable to expect that people may still want to
>> build/test upstream gdb on those?
>
> Damn, I forgot to talk about RHEL.
>
> But yeah, RHEL-6.x is not EOL'ed, but the GDB that comes with it is from
> Red Hat as well, and contains all the necessary patches to deal with
> whatever "internal idiosyncrasy" that it may need. OTOH, I know Gary
> has been using RHEL-6.5 to test his upstream patches, so to answer your
> question, it is still possible that people may still want to buid/test
> upstream GDB there.
>
> My opinion is that we shouldn't need to worry about internals of each
> distro (I know we *have to do that* sometimes, but that's not an
> excuse), so I still hold the cleanup patch for approval :-).
There's the system GDB, that is usually maintained by the
distro, but then it's quite often the case that people build
and ship their own tools on top of the distro, bypassing the
system tools.
I tend to view supporting older-ish distros that people might
still be using like the proprietary OSs we "support" (in a sense).
I think that just as we'd accept a patch that makes GDB work better
on Windows 7 OOTB (e.g., to work around some debug API issue), even
though there's already Windows 8 out there, I think patches that make
GDB work better OOTB on a bit older (but still in use) distros are
fine, as long as they don't get in the way of progress and don't
impose a big maintenance burden.
IMHO, there's no harm in leaving this particular bit in
a while longer.
But I certainly won't cry over this. I'm not personally affected.
If others are fine with yanking this out, I'll be fine with it too.
Thanks,
Pedro Alves
On Thursday, September 25 2014, Pedro Alves wrote:
> There's the system GDB, that is usually maintained by the
> distro, but then it's quite often the case that people build
> and ship their own tools on top of the distro, bypassing the
> system tools.
Yeah.
> I tend to view supporting older-ish distros that people might
> still be using like the proprietary OSs we "support" (in a sense).
> I think that just as we'd accept a patch that makes GDB work better
> on Windows 7 OOTB (e.g., to work around some debug API issue), even
> though there's already Windows 8 out there, I think patches that make
> GDB work better OOTB on a bit older (but still in use) distros are
> fine, as long as they don't get in the way of progress and don't
> impose a big maintenance burden.
Heh, in my personal opinion GDB should not support proprietary OSes
OOTB. But I certainly don't want to start a flamewar.
As for support a bit older distro that might still be out there, I
totally agree with you. The problem is that we (as a community) don't
usually track those things very well, and code here tends to be forgot
until someone stumbles on it because of some bug...
> IMHO, there's no harm in leaving this particular bit in
> a while longer.
Me too, definitely, but there's the issue I raised in the sentence
above...
> But I certainly won't cry over this. I'm not personally affected.
> If others are fine with yanking this out, I'll be fine with it too.
Thanks. I will wait a few more days, and if nobody else objects, I will
go ahead and push this patch in.
BTW, if I push this in, I believe my other patch to adjust the testsuite
becomes obvious, right? (Assuming that the test is indeed needed, as
you already pointed out).
Thanks,
On 09/25/2014 10:53 PM, Sergio Durigan Junior wrote:
>> I tend to view supporting older-ish distros that people might
>> still be using like the proprietary OSs we "support" (in a sense).
>> I think that just as we'd accept a patch that makes GDB work better
>> on Windows 7 OOTB (e.g., to work around some debug API issue), even
>> though there's already Windows 8 out there, I think patches that make
>> GDB work better OOTB on a bit older (but still in use) distros are
>> fine, as long as they don't get in the way of progress and don't
>> impose a big maintenance burden.
>
> Heh, in my personal opinion GDB should not support proprietary OSes
> OOTB. But I certainly don't want to start a flamewar.
I don't either. But I'd rather a user stuck on such a OS be able to
use a free debugger, than drive him towards a proprietary debugger.
That's part of how I got involved into GDB in the first place. I was
forced to used Windows at work. I worked around that by using Cygwin,
to be able to use the free tools I preferred. At the same time
I needed to build a tool that would run on Windows CE. So I worked on
the GNU toolchain in order to target that OS. Then I wanted to make Cygwin
GDB better too, because it was similar to CE, and I was using it
at work too. And then somehow I ended up working on GDB full
time. :-P It's a trap, I tells ya!
The real point was that the user building GDB may have no control
over the system bits of the distro it is building GDB for (in this
case glibc's loader), just like when building for a proprietary OS,
even though GNU/Linux distros are based (mostly) on free sources.
Thanks,
Pedro Alves
On Thursday, September 25 2014, Pedro Alves wrote:
> I don't either. But I'd rather a user stuck on such a OS be able to
> use a free debugger, than drive him towards a proprietary debugger.
> That's part of how I got involved into GDB in the first place. I was
> forced to used Windows at work. I worked around that by using Cygwin,
> to be able to use the free tools I preferred. At the same time
> I needed to build a tool that would run on Windows CE. So I worked on
> the GNU toolchain in order to target that OS. Then I wanted to make Cygwin
> GDB better too, because it was similar to CE, and I was using it
> at work too. And then somehow I ended up working on GDB full
> time. :-P It's a trap, I tells ya!
Haha, thanks for sharing your experience :-).
> The real point was that the user building GDB may have no control
> over the system bits of the distro it is building GDB for (in this
> case glibc's loader), just like when building for a proprietary OS,
> even though GNU/Linux distros are based (mostly) on free sources.
Yeah, that is a fair point, and it is very valid when we talk about
things that might make GDB break badly when removed. But in this case,
we are talking about a very specific Fedora/RHEL thing, which is itself
intended to improve something (i.e., GDB will still work without it on
old Fedora/RHEL systems), so I think most of the concerns don't apply
here.
Sergio Durigan Junior wrote:
> On Thursday, September 25 2014, Pedro Alves wrote:
> > The real point was that the user building GDB may have no control
> > over the system bits of the distro it is building GDB for (in this
> > case glibc's loader), just like when building for a proprietary
> > OS, even though GNU/Linux distros are based (mostly) on free
> > sources.
>
> Yeah, that is a fair point, and it is very valid when we talk about
> things that might make GDB break badly when removed. But in this
> case, we are talking about a very specific Fedora/RHEL thing, which
> is itself intended to improve something (i.e., GDB will still work
> without it on old Fedora/RHEL systems), so I think most of the
> concerns don't apply here.
The code is dead, but only recently so. It's still warm :)
Removing the prefixed-probes code would reintroduce this:
https://sourceware.org/bugzilla/show_bug.cgi?id=2328
My preference is to leave the prefixes in, at least for now; you don't
know what people are using out there. The comment lists affected OS
versions so future reviewers will be able to decide how relevant it
is.
I don't think we need to bother with prefix support in the testsuite,
but we should probably switch to a probe that GDB is using like Pedro
mentioned earlier. "reloc_complete" would be good.
Cheers,
Gary
@@ -1983,71 +1983,47 @@ svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
os = find_pc_section (address);
if (os != NULL)
{
- int with_prefix;
+ VEC (probe_p) *probes[NUM_PROBES];
+ int all_probes_found = 1;
+ int checked_can_use_probe_arguments = 0;
+ int i;
- for (with_prefix = 0; with_prefix <= 1; with_prefix++)
+ memset (probes, 0, sizeof (probes));
+ for (i = 0; i < NUM_PROBES; i++)
{
- VEC (probe_p) *probes[NUM_PROBES];
- int all_probes_found = 1;
- int checked_can_use_probe_arguments = 0;
- int i;
-
- memset (probes, 0, sizeof (probes));
- for (i = 0; i < NUM_PROBES; i++)
- {
- const char *name = probe_info[i].name;
- struct probe *p;
- char buf[32];
-
- /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
- shipped with an early version of the probes code in
- which the probes' names were prefixed with "rtld_"
- and the "map_failed" probe did not exist. The
- locations of the probes are otherwise the same, so
- we check for probes with prefixed names if probes
- with unprefixed names are not present. */
- if (with_prefix)
- {
- xsnprintf (buf, sizeof (buf), "rtld_%s", name);
- name = buf;
- }
+ const char *name = probe_info[i].name;
+ struct probe *p;
+ char buf[32];
- probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
+ probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
- /* The "map_failed" probe did not exist in early
- versions of the probes code in which the probes'
- names were prefixed with "rtld_". */
- if (strcmp (name, "rtld_map_failed") == 0)
- continue;
+ if (VEC_empty (probe_p, probes[i]))
+ {
+ all_probes_found = 0;
+ break;
+ }
- if (VEC_empty (probe_p, probes[i]))
+ /* Ensure probe arguments can be evaluated. */
+ if (!checked_can_use_probe_arguments)
+ {
+ p = VEC_index (probe_p, probes[i], 0);
+ if (!can_evaluate_probe_arguments (p))
{
all_probes_found = 0;
break;
}
-
- /* Ensure probe arguments can be evaluated. */
- if (!checked_can_use_probe_arguments)
- {
- p = VEC_index (probe_p, probes[i], 0);
- if (!can_evaluate_probe_arguments (p))
- {
- all_probes_found = 0;
- break;
- }
- checked_can_use_probe_arguments = 1;
- }
+ checked_can_use_probe_arguments = 1;
}
+ }
- if (all_probes_found)
- svr4_create_probe_breakpoints (gdbarch, probes, os->objfile);
+ if (all_probes_found)
+ svr4_create_probe_breakpoints (gdbarch, probes, os->objfile);
- for (i = 0; i < NUM_PROBES; i++)
- VEC_free (probe_p, probes[i]);
+ for (i = 0; i < NUM_PROBES; i++)
+ VEC_free (probe_p, probes[i]);
- if (all_probes_found)
- return;
- }
+ if (all_probes_found)
+ return;
}
create_solib_event_breakpoint (gdbarch, address);