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 CC6E2384D9A4 for ; Thu, 24 Nov 2022 10:46:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CC6E2384D9A4 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=1669286810; 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=Uarcm+jn6QtCjeZcw10xGPivsAJ8AD+I5Q6UiKru/kc=; b=LEH58Nuuk6XR8zq1mNkuw/CWQnSgRrzvz41fFDQRtjf/zDs/cvF0auAtoF+zn1AiUEs85M 4mHRzvG0OhQ9tGDmfKawW0rtYpiocKXABKx7JAwkXyxwR84EEkGvcNnLCx1VSJG888OxDa 56bL5/eTRBnL4Wl2WztkhuuuwPyy1I0= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-280-nuLdcb7JNcSA8ASABVA1eg-1; Thu, 24 Nov 2022 05:46:49 -0500 X-MC-Unique: nuLdcb7JNcSA8ASABVA1eg-1 Received: by mail-wr1-f70.google.com with SMTP id i24-20020adfa518000000b00241e2f40d8bso303941wrb.14 for ; Thu, 24 Nov 2022 02:46:48 -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=Uarcm+jn6QtCjeZcw10xGPivsAJ8AD+I5Q6UiKru/kc=; b=xjDHYuZ9ALLGjxROy1TSn8BKd2WNTCnRhgukkuxyO0FKZ63VJrhyEYZSah30vK08kb tgRvtEpyNRO/1O/4/COcICOxuHow7byUvvRVi5W7zVM5A5DHYuHNLHA7ZRY1SRRoh62S 4UiFo+pXynC39z5jI68ZUNTTfSsaWkGm5w4lKSEuYu4COdMZ2FNtVhnUH0v1nxhr98p1 rqUL+3AvQUtuYt0RGvK2DlQteqRvyOssGxc8ZPKxSpgmj+tQRpLAiO3maag6dmrEgpWn 8HXlY9WSZJd/4/YYS1vL35qm1QYOm6CVzlq1Ti7pxFlnWHYFTZoTp3LM4Sg1bUxXkrAS 8wAQ== X-Gm-Message-State: ANoB5pm8XgZFgtzYepab5g5V1505N9xZHy7lPfTUqfaitV5DOJn4j+e/ LGJlw3siY2ytJ5xs8xKakIGk1H3oqO5gGEdVjJ6eT8zGIurETEeLdvzCV+wxiwed4I0CghgVn9v s1Nuht0ZCjKnuSdtMJgEDSw== X-Received: by 2002:a5d:4b8c:0:b0:241:e1c6:7e0a with SMTP id b12-20020a5d4b8c000000b00241e1c67e0amr7349022wrt.463.1669286807597; Thu, 24 Nov 2022 02:46:47 -0800 (PST) X-Google-Smtp-Source: AA0mqf7X39BR+DWJo8Qs1rKlwnXg927uGEzwzx/8tvVYAOzOrPNBn5iHV5KlDOVBmOqgvozeGF7zCQ== X-Received: by 2002:a5d:4b8c:0:b0:241:e1c6:7e0a with SMTP id b12-20020a5d4b8c000000b00241e1c67e0amr7349001wrt.463.1669286807218; Thu, 24 Nov 2022 02:46:47 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id u13-20020a5d514d000000b002365b759b65sm999657wrt.86.2022.11.24.02.46.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Nov 2022 02:46:46 -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: <9b1fe2f9-b4a9-17a8-1175-a1a776db1afc@simark.ca> References: <9b1fe2f9-b4a9-17a8-1175-a1a776db1afc@simark.ca> Date: Thu, 24 Nov 2022 10:46:45 +0000 Message-ID: <87y1s03a0a.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: 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])