From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 58F1C3858422 for ; Mon, 28 Nov 2022 17:18:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 58F1C3858422 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669655922; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=yv52Ai/18x5dTdtRzisT3nOoe7ZLH+A0mXSJ5mnWwFU=; b=ht1ikEAhuPTDSuHhzmsuojR/a4K++spIl+SYitj9SEoQ45n/ztuk29VltfRWZ2M8l9gE92 PEtViEUpfWcJpJjeBKfJxJIfWtEaSucSPFqUkNMbyVmBJGBQNb8OijeLelZgOw5EZVjo0H 6y67DX0gf240KtzcdekfmyUFrBRM+2I= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-408-LmtxVdSINo6Yo9O2aIMyHQ-1; Mon, 28 Nov 2022 12:18:40 -0500 X-MC-Unique: LmtxVdSINo6Yo9O2aIMyHQ-1 Received: by mail-wm1-f70.google.com with SMTP id h9-20020a1c2109000000b003cfd37aec58so6545319wmh.1 for ; Mon, 28 Nov 2022 09:18:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=yv52Ai/18x5dTdtRzisT3nOoe7ZLH+A0mXSJ5mnWwFU=; b=dC6H71PDViHMEXjd1Wgvy8VbsgPpYp9/AZ3EKMkijbv+vIFhLYf1vvGyw/38DCVZLf 7Yy4UnfsPdTLDUxO+x4gBiu022LYMhbc0E2MWSNBKZCvzBEmHXJ4a4mfEwgV+Yc0ap9i ZprZRb5iX7TfNzVvZMHiFCiXjcHjw7vey0bEWH9DqEpIWWY5TNTE/TNoy6zluu6ExlLE CLWaXTdXVruHO/xeda7pSUaIuQ9ApCI1QEdbXWi3AAuTR6KVqlcfs6IcAY/3S8IT1f6P Ih2e0MFW0fukF3/fXGO/J7rBC1jahGTiFkvpjYXLGxP0bvPF0iEF3lOh+YD0liAyONav H6vw== X-Gm-Message-State: ANoB5plC9cfoWr0MkQ8TmYUIC1rk3HWOS1bYSBeCfBb2HFatA2gx9vXj c1zAuWKlXM5Ql8WfrupfFqU1TKJS/VG+1UlbPlb4TQHbsrmuhgxvHh4w4BuYlpC0hedsd64LEP/ Xj+tVk4tDvimJuOcECIVtqQ== X-Received: by 2002:a05:600c:22c4:b0:3cf:71b7:7a41 with SMTP id 4-20020a05600c22c400b003cf71b77a41mr39729027wmg.31.1669655919080; Mon, 28 Nov 2022 09:18:39 -0800 (PST) X-Google-Smtp-Source: AA0mqf7dpw/Y+7+eZPiEUleNy1nGVs9TO2qqDjJenzeCJDM0HVWxIK1r61xNyXke5M4J1/uIBdTqag== X-Received: by 2002:a05:600c:22c4:b0:3cf:71b7:7a41 with SMTP id 4-20020a05600c22c400b003cf71b77a41mr39728989wmg.31.1669655918732; Mon, 28 Nov 2022 09:18:38 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id p13-20020a05600c358d00b003c6b9749505sm21015530wmq.30.2022.11.28.09.18.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 09:18:38 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: relax requirement for the map_failed stap probe to be present In-Reply-To: <87y1s03a0a.fsf@redhat.com> References: <9b1fe2f9-b4a9-17a8-1175-a1a776db1afc@simark.ca> <87y1s03a0a.fsf@redhat.com> Date: Mon, 28 Nov 2022 17:18:37 +0000 Message-ID: <87sfi30zgy.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: Thanks you all for your review feedback. I've now pushed this version of the patch (inc Pedro's suggested typo fix). Thanks, Andrew Andrew Burgess writes: > 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. > > 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. > > [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 > + 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), > + then we will still use the probes interface, even if that > + probe is missing. > + > + The only (possible) downside of this is that, if the user has > + 'set stop-on-solib-events on' in effect, then they might get > + fewer events using the probes interface than with the classic > + non-probes interface. */ > + if (prove_info[i].action == DO_NOTHING) > + continue; > + else > + return false; > + } > > /* Ensure probe arguments can be evaluated. */ > for (probe *p : probes[i])