From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id GHvCLDGT9mD9UQAAWB0awg (envelope-from ) for ; Tue, 20 Jul 2021 05:11:13 -0400 Received: by simark.ca (Postfix, from userid 112) id B52EE1EDF2; Tue, 20 Jul 2021 05:11:13 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D89A51E54D for ; Tue, 20 Jul 2021 05:11:11 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 756CE3987027 for ; Tue, 20 Jul 2021 09:11:11 +0000 (GMT) Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) by sourceware.org (Postfix) with ESMTPS id DB2F839874EC for ; Tue, 20 Jul 2021 09:10:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DB2F839874EC Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wm1-x32d.google.com with SMTP id b14-20020a1c1b0e0000b02901fc3a62af78so1050630wmb.3 for ; Tue, 20 Jul 2021 02:10:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=2Epu2yKEYe+35eOTx3/DbhjMqX/tSb55SCMaXnb+vNI=; b=c9nnxOZwlCLxs/pWPM6wZYiybPJxgZi1nNP2BPnhw0bZa4Cp1T4xYV3fhHRnplgs1H VYSq3FiZflyPbEMAF7yIm8cJQCmt49+s16e1j9/ET3SBoujAISAaxPZyRP010DQ17Hth T4BMj+pbhM/2a8QGmEV9c9GrPi/cFJMIgwE8+z19UtjuHFwq/yJkbA1myeXQuDRbLQ8u iTOYtoi4mWv/VhJTNuVlM8MHQ/084VLDwiTzDKUQ0yz+PQxC8EhOBK3qzpN06pjssMso 4AP4rW9YzxuQtUOGJpgyqwgAX6yu5Rcj9dyu7zTZqTRw3+pCtRQfU5QYSh4zb5DrNTns Ki7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=2Epu2yKEYe+35eOTx3/DbhjMqX/tSb55SCMaXnb+vNI=; b=fMty8kDv8bP+f6/bdhMEz+R9T3YvddV+yOz4H/We7hL9IWlZlBZZ1nd2+7MArtmx6X bny+3FrO+ihLNLZbZp9OroQHv+QBfNTeR1geedTphEBcVmWKH4oRwqCMurW72O31EKG2 OtmCsNpdZr3OCjVfRiE9iDoT+/mDvfkgg4KLiq8HXFEJf9Q0zjQHVUV/+6QupnVvFMcG Fr0R/idnveC15+SMx033ka8WaCF8TLAVk52Y6ESLF28ykKQ2d1L+LJxsyphSL3OmH5o1 mJU6cy0ehZdpB/JCSDL1IwFRFukFohGdtNoxTMiS1s0LUeYNqR8znTcmCBkcR1NsugDt lG2g== X-Gm-Message-State: AOAM532ElZBIb4/AILQY6j204AvcgleOeN1mKKCENS39mVGeKfMPinHX JSz+YdNtIbpQpQ58mdi7aZGEl5iaMSLGTg== X-Google-Smtp-Source: ABdhPJwRMoo+HltUvPMRxTNCk7sLjKs+kp+SXy7GaKTd7O0lS+wXmkUGzHDzrfYaB468hlLgaTs9Pw== X-Received: by 2002:a05:600c:1c1f:: with SMTP id j31mr36181866wms.132.1626772231759; Tue, 20 Jul 2021 02:10:31 -0700 (PDT) Received: from localhost (host109-151-14-57.range109-151.btcentralplus.com. [109.151.14.57]) by smtp.gmail.com with ESMTPSA id r19sm24565097wrg.74.2021.07.20.02.10.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Jul 2021 02:10:31 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCHv3 2/2] gdb: remove VALUE_FRAME_ID Date: Tue, 20 Jul 2021 10:10:24 +0100 Message-Id: <39682fd10369b49d1d3996dc266b6608b16dba4c.1626772086.git.andrew.burgess@embecosm.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" While working on the previous commit I happened to switch on 'set debug frame 1', and ran into a different assertion than the one I was trying to fix. The new problem I see is this assertion triggering: gdb/frame.c:622: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->this_id.p != frame_id_status::COMPUTING' failed. We attempt to get the frame_id for a frame while we are computing the frame_id for that same frame. What happens is we have a stack like this: normal_frame -> inline_frame -> sentinel_frame When we initially stop, GDB creates a frame for inline_frame but doesn't sniff its type, or try to fill in its frame_id (see code near the top of get_prev_frame_if_no_cycle). Later on during the stop, this happens: process_event_stop_test get_stack_frame_id skip_artificial_frames get_frame_type The call to get_frame_type causes inline_frame to sniff its type, establishing that it is an INLINE_FRAME, but does not cause the frame to build its frame_id. The same skip_artificial_frames call then calls get_prev_frame_always to unwind the stack, this will then try to get the frame previous to inline_frame, i.e. normal_frame. So, we create a new frame, but unlike frame #0, in get_prev_frame_if_no_cycle, we immediately try to compute the frame_id for this new frame #1. Computing the frame_id for frame #1 invokes the sniffer. If this sniffer tries to read a register then we will create a lazy register value by calling value_of_register_lazy. As the next frame (frame #0) is an INLINE_FRAME, we will skip this and instead create the lazy register value using the frame_id for frame #-1 (the sentinel frame). Now, when we try to resolve the lazy register value, in the value.c function value_fetch_lazy_register, we want to print which frame the lazy register value was for (in debug mode), so we call: frame = frame_find_by_id (VALUE_FRAME_ID (val)); where: #define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val))) In our case we call get_prev_frame_id_by_id with the frame_id of the sentinel_frame frame, we then call frame_find_by_id, followed by get_prev_frame (which gives us frame #0), followed by get_frame_id. Frame #0 has not yet computed its frame_id, so we start that process. The first thing we need when computing the frame_id of an inline frame is the frame_id of the previous frame, so that's what we ask for. Unfortunately, that's where we started this journey, we are already computing the frame_id for frame #1, and thus the assert fires. Solving the assertion failure is pretty easy, if we consider the code in value_fetch_lazy_register and get_prev_frame_id_by_id then what we do is: 1. Start with a frame_id taken from a value, 2. Lookup the corresponding frame, 3. Find the previous frame, 4. Get the frame_id for that frame, and 5. Lookup the corresponding frame 6. Print the frame's level Notice that steps 3 and 5 give us the exact same result, step 4 is just wasted effort. We could shorten this process such that we drop steps 4 and 5, thus: 1. Start with a frame_id taken from a value, 2. Lookup the corresponding frame, 3. Find the previous frame, 6. Print the frame's level This will give the exact same frame as a result, and this is what I have done in this patch by removing the use of VALUE_FRAME_ID from value_fetch_lazy_register. Out of curiosity I looked to see how widely VALUE_FRAME_ID was used, and saw it was only used in one other place in valops.c:value_assign, where, once again, we take the result of VALUE_FRAME_ID and pass it to frame_find_by_id, thus introducing a redundant frame_id lookup. I don't think the value_assign case risks triggering the assertion though, as we are unlikely to call value_assign while computing the frame_id for a frame, however, we could make value_assign slightly more efficient, with no real additional complexity, by removing the use of VALUE_FRAME_ID. So, in this commit, I completely remove VALUE_FRAME_ID, and replace it with a use of VALUE_NEXT_FRAME_ID, followed by a direct call to get_prev_frame_always, this should make no difference in either case, and resolves the assertion issue from value.c. One thing that is worth noticing here is that in these two situations we don't end up getting the frame we expected to, though this is not a result of my change, we were not getting the expected result before either. Consider the debug printing case, the stack is: normal_frame -> inline_frame -> sentinel_frame We read a register from normal_frame (frame #1), the value of which is fetched from sentinel_frame (frame #-1). The debug print is trying to say: frame=1,regnum=.... However, as the lazy register value points at frame #-1, we will actually (incorrectly) print: frame=0,regnum=.... Like I said, this bug existed before this commit, and so, if we didn't assert (i.e. the lazy register read occurred in some context other than during the frame sniffer), then the debug print would previous have, and will continue to, print the wrong frame level. Thankfully, this is only in debug output, so not something a normal user should see. In theory, the same bug exists in the value_assign code, if we are in normal_frame and try to perform a register assignment, then GDB will get confused and think we are assigning in the context of inline_frame. However, having looked at the code I think we get away with this as the frame is used for two things that I can see: 1. Getting the gdbarch for the frame, I can't imagine a situation where inline_frame has a different gdbarch to normal_frame, and 2. Unwinding register values from frame->next. We should be asking to unwind the register values from inline_frame, but really we end up asking to unwind from sentinel_frame. However, if we did ask to unwind the values from inline_frame this would just forward the request on to the next frame, i.e. sentinel_frame, so we would get the exact same result. In short, though we do use the wrong frame in value_assign, I think this is harmless. Fixing this debug printing would require GDB to require extra information in its value location to indicate how many frames had been skipped. For example, with the stack: normal_frame -> inline_frame -> sentinel_frame A lazy register value read in frame normal_frame would have a location frame_id for sentinel_frame, and a skip value of 2 indicating the value was read for a frame 2 previous. In contrast, for a more standard case, with a stack like this: normal_frame -> sentinel_frame A lazy register value read in frame normal_frame would have a location frame_id for sentinel_frame and a skip value of 1 indicating the value was read for a frame 1 previous. However, adding this seems like a lot of work to fix a single like of debug print, but might be something we want to consider in the future. --- gdb/frame.c | 16 ---------------- gdb/frame.h | 4 ---- .../gdb.base/inline-frame-cycle-unwind.exp | 17 +++++++++++++++++ gdb/valops.c | 17 +++++++++-------- gdb/value.c | 5 ++--- gdb/value.h | 6 ------ 6 files changed, 28 insertions(+), 37 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index 4f612d3546b..45a34badec6 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2552,22 +2552,6 @@ get_prev_frame (struct frame_info *this_frame) return get_prev_frame_always (this_frame); } -struct frame_id -get_prev_frame_id_by_id (struct frame_id id) -{ - struct frame_id prev_id; - struct frame_info *frame; - - frame = frame_find_by_id (id); - - if (frame != NULL) - prev_id = get_frame_id (get_prev_frame (frame)); - else - prev_id = null_frame_id; - - return prev_id; -} - CORE_ADDR get_frame_pc (struct frame_info *frame) { diff --git a/gdb/frame.h b/gdb/frame.h index 0d2bc08a47b..2548846c1ed 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -394,10 +394,6 @@ extern struct frame_info *get_prev_frame_always (struct frame_info *); is not found. */ extern struct frame_info *frame_find_by_id (struct frame_id id); -/* Given a frame's ID, find the previous frame's ID. Returns null_frame_id - if the frame is not found. */ -extern struct frame_id get_prev_frame_id_by_id (struct frame_id id); - /* Base attributes of a frame: */ /* The frame's `resume' address. Where the program will resume in diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp index 27709fe2232..2801b683a03 100644 --- a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp @@ -126,3 +126,20 @@ with_test_prefix "cycle at level 1" { "#1 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ "Backtrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"] } + +# Flush the register cache (which also flushes the frame cache) so we +# get a full backtrace again, then switch on frame debugging and try +# to back trace. At one point this triggered an assertion. +gdb_test "maint flush register-cache" \ + "Register cache flushed\\." "" +gdb_test_no_output "set debug frame 1" +gdb_test_multiple "bt" "backtrace with debugging on" { + -re "^$gdb_prompt $" { + pass $gdb_test_name + } + -re "\[^\r\n\]+\r\n" { + exp_continue + } +} +gdb_test "p 1 + 2 + 3" " = 6" \ + "ensure GDB is still alive" diff --git a/gdb/valops.c b/gdb/valops.c index bd547923496..50874a5f55d 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1197,14 +1197,15 @@ value_assign (struct value *toval, struct value *fromval) struct gdbarch *gdbarch; int value_reg; - /* Figure out which frame this is in currently. - - We use VALUE_FRAME_ID for obtaining the value's frame id instead of - VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed to - put_frame_register_bytes() below. That function will (eventually) - perform the necessary unwind operation by first obtaining the next - frame. */ - frame = frame_find_by_id (VALUE_FRAME_ID (toval)); + /* Figure out which frame this register value is in. The value + holds the frame_id for the next frame, that is the frame this + register value was unwound from. + + Below we will call put_frame_register_bytes which requires that + we pass it the actual frame in which the register value is + valid, i.e. not the next frame. */ + frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (toval)); + frame = get_prev_frame_always (frame); value_reg = VALUE_REGNUM (toval); diff --git a/gdb/value.c b/gdb/value.c index 6a07495d32b..91db66fa3be 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -3950,9 +3950,8 @@ value_fetch_lazy_register (struct value *val) { struct gdbarch *gdbarch; struct frame_info *frame; - /* VALUE_FRAME_ID is used here, instead of VALUE_NEXT_FRAME_ID, - so that the frame level will be shown correctly. */ - frame = frame_find_by_id (VALUE_FRAME_ID (val)); + frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (val)); + frame = get_prev_frame_always (frame); regnum = VALUE_REGNUM (val); gdbarch = get_frame_arch (frame); diff --git a/gdb/value.h b/gdb/value.h index 379cddafbe7..e1c6aabfa29 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -458,12 +458,6 @@ extern struct internalvar **deprecated_value_internalvar_hack (struct value *); extern struct frame_id *deprecated_value_next_frame_id_hack (struct value *); #define VALUE_NEXT_FRAME_ID(val) (*deprecated_value_next_frame_id_hack (val)) -/* Frame ID of frame to which a register value is relative. This is - similar to VALUE_NEXT_FRAME_ID, above, but may not be assigned to. - Note that VALUE_FRAME_ID effectively undoes the "next" operation - that was performed during the assignment to VALUE_NEXT_FRAME_ID. */ -#define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val))) - /* Register number if the value is from a register. */ extern int *deprecated_value_regnum_hack (struct value *); #define VALUE_REGNUM(val) (*deprecated_value_regnum_hack (val)) -- 2.25.4