From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id C06233830B13 for ; Thu, 24 Nov 2022 15:10:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C06233830B13 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 385E11E11A; Thu, 24 Nov 2022 10:10:24 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1669302624; bh=/2MVD25FYYWCCjqYrIj2hOUwn+VKjkNdQqAPYqdpgKc=; h=Date:Subject:To:References:From:In-Reply-To:From; b=YHrzCO82CBp1RFD+NdcycW428lF0gqux9gabZW803XCMXOOuxGAZ5ESzMh2Y4Nfur HZ7r50xopKE5U/zbK9AYyz4K2paIpTGVBfPadzOlN/AHJtCoWzVLcYIJwAq1li88SZ 07qTyEWYMmKqOjcK3Y4hqNWKH4ilZ4y7mzijxc6U= Message-ID: <92c736bb-d188-ff07-f3cc-71d63fb0f3ae@simark.ca> Date: Thu, 24 Nov 2022 10:10:23 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH] gdb: relax requirement for the map_failed stap probe to be present Content-Language: fr To: Andrew Burgess , gdb-patches@sourceware.org References: <9b1fe2f9-b4a9-17a8-1175-a1a776db1afc@simark.ca> <87y1s03a0a.fsf@redhat.com> From: Simon Marchi In-Reply-To: <87y1s03a0a.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/24/22 05:46, Andrew Burgess via Gdb-patches wrote: > Simon Marchi writes: > >> On 11/22/22 10:09, Andrew Burgess via Gdb-patches wrote: >>> From glibc 2.35 and later, the "map_failed" stap probe is no longer >>> included in glibc. The removal of the probe looks like an accident, >>> but it was caused by a glibc commit which meant that the "map_failed" >>> probe could no longer be reached; the compiler than helpfully >>> optimised out the probe. >>> >>> In GDB, in solib-svr4.c, we have a list of probes that we look for >>> related to the shared library loading detection. If any of these >>> probes are missing then GDB will fall back to the non-probe based >>> mechanism for detecting shared library loading. The "map_failed" >>> probe is include in the list of required probes. >>> >>> This means that on glibc 2.35 (or later) systems, GDB is going to >>> always fall back to the non-probes based mechanism for detecting >>> shared library loading. >>> >>> I raised a glibc bug to discuss this issue: >>> >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29818 >>> >>> But, whatever the ultimate decision from the glibc team, given there >>> are version of glibc in the wild without the "map_failed" probe, we >>> probably should update GDB to handle this situation. >>> >>> The "map_failed" probe is already a little strange, very early >>> versions of glibc didn't include this probe, so, in some cases, if >>> this probe is missing GDB is happy to ignore it. In this commit I >>> just expand this logic to make the "map_failed" probe fully optional. >>> >>> With this commit in place, then, when using a glibc 2.35 or later >>> system, GDB will once again use the stap probes for shared library >>> detection. >>> --- >>> gdb/solib-svr4.c | 13 +++++++++---- >>> 1 file changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c >>> index 6acaf87960b..87cd06f251a 100644 >>> --- a/gdb/solib-svr4.c >>> +++ b/gdb/solib-svr4.c >>> @@ -2205,10 +2205,15 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info, >>> >>> 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 (with_prefix && streq (name, "rtld_map_failed")) >>> + /* The "map_failed" probe did not exist in early versions of the >>> + probes code in which the probes' names were prefixed with >>> + "rtld_". >>> + >>> + Additionally, the "map_failed" probe was accidentally removed >>> + from glibc 2.35 and later, when changes in glibc meant the probe >>> + could no longer be reached. In this case the probe name doesn't >>> + have the "rtld_" prefix. */ >>> + if (streq (probe_info[i].name, "map_failed")) >>> continue; >>> >>> /* Ensure at least one probe for the current name was found. */ >>> >>> base-commit: 84f9fbe90e5429adb9dee68f04f44c92fa9e2345 >>> -- >>> 2.25.4 >> >> Hi, >> >> I looked at this separately, and this was one of the fixes I considered. >> >> Another option was to make GDB not give up on the probes interface if >> failing to look up a probe whose action is DO_NOTHING. Probes with that >> action are not used by GDB for solib bookkeeping, but can be used to >> stop on solib events, with "set stop-on-solib-events". I was just >> worried if there was some cases where a probe would be missing, but the >> corresponding event could be caught if using the original interface. In >> that case, using the probes interface would be a regression. But it's >> probably not worth wondering about. If that happens it's just a bug >> that needs to be fixed. In the case we are looking at, if the >> map_failed probe gets optimized out, then surely the corresponding call >> to the r_brk function would also be optimized out. > > I also considered just ignoring any probe was (a) missing, and (b) had a > DO_NOTHING action. The reason I didn't post this patch was because, at > the time, my thinking was: if we don't care about any probe with a > DO_NOTHING action, why even look for those probes, why not just remove > them from the list? > > I think you've (partially) convinced me that the user might be > interested in seeing a stop at these probes even if GDB's action is > DO_NOTHING. > > I say partially above because GDB doesn't really do anything to tell the > user which probe we stopped at, e.g. was is "init_start", "map_start", > "map_failed", etc. The user might be able to figure it out from the > backtrace, but I still think it's not going to be trivial in all cases, > e.g. "map_start" and "map_failed" are both located in the same function, > so I think the user would need to lookup the probe address in the ELF, > then compare that to the stop address. Not impossible, but, I suspect, > the complexity is an indication that users are not doing this much. > Thus, I suspect, in reality, nobody really cares about the DO_NOTHING > probes. That's a good point. > However, I think there is enough of a justification there for keeping > the probes in the list, and just skipping any DO_NOTHING probes that > don't exist. > > Below then, is an alternative patch. I don't have a strong preference > between this one, and the original[1], but I thought I'd post this for > discussion. If this is preferred then I can just merge this. You convinced me that this is fine, I think I am really over-worrying. I like this version of the patch. > [1] I'll also post an update to the original patch shortly that > addresses Lancelot's feedback. > > Thanks, > Andrew > > --- > > commit 11be1f25f446e68c23d0709cde46e32ff24b7eb9 > Author: Andrew Burgess > Date: Tue Nov 22 12:45:56 2022 +0000 > > gdb: relax requirement for the map_failed stap probe to be present > > From glibc 2.35 and later, the "map_failed" stap probe is no longer > included in glibc. The removal of the probe looks like an accident, > but it was caused by a glibc commit which meant that the "map_failed" > probe could no longer be reached; the compiler than helpfully > optimised out the probe. > > In GDB, in solib-svr4.c, we have a list of probes that we look for > related to the shared library loading detection. If any of these > probes are missing then GDB will fall back to the non-probe based > mechanism for detecting shared library loading. The "map_failed" > probe is include in the list of required probes. > > This means that on glibc 2.35 (or later) systems, GDB is going to > always fall back to the non-probes based mechanism for detecting > shared library loading. > > I raised a glibc bug to discuss this issue: > > https://sourceware.org/bugzilla/show_bug.cgi?id=29818 > > But, whatever the ultimate decision from the glibc team, given there > are version of glibc in the wild without the "map_failed" probe, we > probably should update GDB to handle this situation. > > The "map_failed" probe is already a little strange, very early > versions of glibc didn't include this probe, so, in some cases, if > this probe is missing GDB is happy to ignore it. This is fine, the > action associated with this probe inside GDB is DO_NOTHING, this means > the probe isn't actually required in order for GDB to correctly detect > the loading of shared libraries. > > In this commit I propose changing the rules so that any probe whose > action is DO_NOTHING, is optional. > > There is one possible downside to this change, and that concerns 'set > stop-on-solib-events on'. If a probe is removed from glibc, but the > old style breakpoint based mechanism is still in place within glibc > for that same event, then GDB will stop when using the old style > non-probe based mechanism, but not when using the probes based > mechanism. > > For the map_failed case this is not a problem, both the map_failed > probe, and the call to the old style breakpoint location were > optimised out, and so neither event (probes based, or breakpoint > based) will trigger. This would only become an issue if glibc removed > a probe, but left the breakpoint in place (this would almost certainly > be a bug in glibc). > > For now, I'm proposing that we just don't worry about this. Because > some probes have actions that are not DO_NOTHING, then we know the > user will always seem _some_ stops when a shared library is > loaded/unloaded, and (I'm guessing), in most cases, that's all they > care about. I figure when someone complains then we can figure out > what the right solution is then. > > With this commit in place, then, when using a glibc 2.35 or later > system, GDB will once again use the stap probes for shared library > detection. > > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c > index 6acaf87960b..10e446af908 100644 > --- a/gdb/solib-svr4.c > +++ b/gdb/solib-svr4.c > @@ -2205,15 +2205,34 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info, > > 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 (with_prefix && streq (name, "rtld_map_failed")) > - continue; > - > /* Ensure at least one probe for the current name was found. */ > if (probes[i].empty ()) > - return false; > + { > + /* The "map_failed" probe did not exist in early versions of the > + probes code in which the probes' names were prefixed with > + "rtld_". > + > + Additionally, the "map_failed" probe was accidentally removed > + from glibc 2.35 and later, when changes in glibc meant the Nit: I would maybe just say "removed in glibc 2.35" and not imply anything about later versions, as we don't know whether it will be there or not. > + probe could no longer be reached, and the compiler optimized > + the probe away. In this case the probe name doesn't have the > + "rtld_" prefix. > + > + To handle this, and give GDB as much flexibility as possible, > + we make the rule that, if a probe isn't required for the > + correct operation of GDB (i.e. it's action is DO_NOTHING), it's -> its, I think Simon