From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id KGsiIruqsmCyBAAAWB0awg (envelope-from ) for ; Sat, 29 May 2021 16:57:31 -0400 Received: by simark.ca (Postfix, from userid 112) id 81FF71F11E; Sat, 29 May 2021 16:57:31 -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.5 required=5.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,RDNS_DYNAMIC,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.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 3E34B1E813 for ; Sat, 29 May 2021 16:57:27 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DB7A53886C79; Sat, 29 May 2021 20:57:26 +0000 (GMT) Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id 03AEA388A03C for ; Sat, 29 May 2021 20:57:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 03AEA388A03C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x436.google.com with SMTP id f11so6742622wrq.1 for ; Sat, 29 May 2021 13:57:23 -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=kPsHeDbRn0KYUgAXgg2apYMrzfNQKkvujGTv9wZWJhk=; b=Xyg/Y7Pgj0hawgPucmqn0q9SXP6ZTAzJIogfGgQ955VYKmV/Kv258qjSYF4pmevk3Y EfMd6g/MoylGV5jnW+HxwHURk9lFksLdmRmbUJOeKd0L6MAiXjYJ/niq5cH/th0LXgdC riytzSKVVwDkvF5Tcv7salewkvjU2DqcSfVKJ33wkGTpyaUhFHQ71CYfG5PaUp2FtmnX Z6FaRQVSAmmMwO9Zofjj6k2z55kSH/CEXJuyBB4rwk574czQkcoiHlKNawQ3l2SfR8X1 tTYHBULsM/UmzOUPRHBsoeYWn4sxmJqacGkIPEpBdRMmI51pdvsTRPnVOnHOBHTlfXD2 H0fA== 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=kPsHeDbRn0KYUgAXgg2apYMrzfNQKkvujGTv9wZWJhk=; b=EqkCnV2noy8IOQzaDYxG5RSsoitjnW/EMBKSTspCTv61IkGWAYTKTn9A/+uJzKu36Y Ic+HHbrxJ0JR8MGwTo6nY3Lcz+QEWz8QkLV0uplREqfThyR3gmecn6eR3XQTPdciJCwK X4Q5qvLGPP10e4+upmXollZuPNTdlE8zOW/UlYkQw1Io44p7QBYoCBgtiqnYiv6/3L96 RMVGwHmxhmQDyfXuM61lRD4gM2KGjGA0frbZGOztEDlRYaqYUY5WZSaTQeNuoyQCRL1O 8+mhk0ZtRVJR2oaO+5CJjClNsHKzy7OihOZOWp/qVQARSCh1h/mY+0sNWHw0zzSDUAqu AYjw== X-Gm-Message-State: AOAM531XqVHbOlIxGIGxTa9fbw2DWPVSFDr2opZ8IlF7rQXaEa79IYYz WFJJF8asYNOSGfLlbRoEm3TA5XKWGJq5ZA== X-Google-Smtp-Source: ABdhPJwD4YAUrhNx22GXnTJQIauQduDM8fTkBeRE/c8vgHOg+wWqP1Yl0N5UZavarVNGuBmdBDzuTw== X-Received: by 2002:adf:fa46:: with SMTP id y6mr6596386wrr.194.1622321842733; Sat, 29 May 2021 13:57:22 -0700 (PDT) Received: from localhost (host109-151-46-70.range109-151.btcentralplus.com. [109.151.46.70]) by smtp.gmail.com with ESMTPSA id z12sm13125754wmc.5.2021.05.29.13.57.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 29 May 2021 13:57:22 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH 5/5] gdb: remove VALUE_FRAME_ID Date: Sat, 29 May 2021 21:57:14 +0100 Message-Id: <3a7784f3667c5a8f09a2768ab45fa525ce7ef40f.1622321523.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@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/ChangeLog: * frame.c (get_prev_frame_id_by_id): Delete. * frame.h (get_prev_frame_id_by_id): Delete declaration. * valops.c (value_assign): Use VALUE_NEXT_FRAME_ID and get_prev_frame_always, not VALUE_FRAME_ID. * value.c (value_fetch_lazy_register): Likewise. * value.h (VALUE_FRAME_ID): Delete. gdb/testsuite/ChangeLog: * gdb.base/inline-frame-bad-unwind.exp: Add an extra test. --- gdb/ChangeLog | 9 +++++++++ gdb/frame.c | 16 ---------------- gdb/frame.h | 4 ---- gdb/testsuite/ChangeLog | 4 ++++ .../gdb.base/inline-frame-bad-unwind.exp | 17 +++++++++++++++++ gdb/valops.c | 17 +++++++++-------- gdb/value.c | 5 ++--- gdb/value.h | 6 ------ 8 files changed, 41 insertions(+), 37 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index b0943c02115..e29a132dc3e 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2631,22 +2631,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 da52522ad2a..bc46149697e 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -383,10 +383,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-bad-unwind.exp b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp index 49c35517801..a0aebf94e41 100644 --- a/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp +++ b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp @@ -120,3 +120,20 @@ gdb_test_sequence "bt" "Backtrace when the unwind is broken at frame 1" { "\\r\\n#1 \[^\r\n\]* bar \\(\\) at " "\\r\\nBacktrace 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 8694c124b52..91832cc6b04 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 9df035a50b3..034cfa9f11c 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 a691f3cf3ff..231387784a8 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