From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id F0B17385482F for ; Tue, 29 Jun 2021 09:27:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F0B17385482F 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-wr1-x42e.google.com with SMTP id j2so24973601wrs.12 for ; Tue, 29 Jun 2021 02:27:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=bqiIglA5zhY1AihhOO9eupFiL7J4+k9dB3oOp4HoKVM=; b=XWPo5UJsHvcbSyPHkqliLjIJ4h+EJV7HfiNyhLRIGqRsjq784oJbC1Rxz20iwK1Wxx TDamx7fIfer6l7hB+Ep9ZXwz/ywpEgwQEU8bC56WznoT8Bk9zZ4Y7XEnFdlO/xyLa0m8 qznbmSIazNofdScJ5qJhdexC0/DvDOldqdNodT7j2AXd1Sunj7ipI5ROmaMwGZTx/7e+ xytUjNwsr7UHVGnhvYljMuajAucAY4xq+syRw5GOuZtGTFPz9m6QXBoGasmTWejr8p+4 U2iQQqIvFIC7U6UhR0FiA5ITydbhH0jP4rqrIoWU7MupgKoTxCh/s5Iu5zUSFVLeLc1X iuSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=bqiIglA5zhY1AihhOO9eupFiL7J4+k9dB3oOp4HoKVM=; b=fr2Tu8k6C4SaB/JLFSCNYdMYeRrXwSX5zJFUo5ba4Ef4S8oC15/Y46ZPJyzA/p+UvY hYn5pnyj4p84KzCK+svMD+P9oaimUUlfBCt7MrblKVdWKfpGaMylG4Jcxg6SzwxP/YyC pRn/XGBeM2xPtmTEK4jnt/1jlqnEWiYYIbqX4o8/9DYAttLsWFx4PIR94D18GSIgwvhd WCGfBjOb1uWNG3knARNkDv4xswnhWRpmefSwxcXgyqZnF5XK7Q1zrc1Bagxzk6t45QM0 abtEyqXzKZEPHtCKdU16U9ehEvzBM++Nsm9W4sLODjmaauK7GpUa6P1NoRuyYCszyQl4 mMvw== X-Gm-Message-State: AOAM533f6URlASh3bcetciz7rMS9nbAHAeiRl+lqNdr98o7laDx9XWa+ wABgykk5vCWe/7I4vYWpkYBUgQ== X-Google-Smtp-Source: ABdhPJznjCwwWxD6yaWQKRaZdUGHSf9vOokMLQmhvdmn3fpuDMSu+qEoG1KVBPWcWUOJbxaXfqhGdQ== X-Received: by 2002:adf:d203:: with SMTP id j3mr12754873wrh.292.1624958855159; Tue, 29 Jun 2021 02:27:35 -0700 (PDT) Received: from localhost (host86-140-92-85.range86-140.btcentralplus.com. [86.140.92.85]) by smtp.gmail.com with ESMTPSA id e12sm17971046wrw.34.2021.06.29.02.27.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Jun 2021 02:27:34 -0700 (PDT) Date: Tue, 29 Jun 2021 10:27:33 +0100 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/5] gdb: value_fetch_lazy_register: fix getting frame id while it is computed Message-ID: <20210629092733.GA2568@embecosm.com> References: <20210628174429.275911-1-simon.marchi@polymtl.ca> <20210628174429.275911-3-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210628174429.275911-3-simon.marchi@polymtl.ca> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 10:22:02 up 12 days, 17:13, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Tue, 29 Jun 2021 09:27:38 -0000 * Simon Marchi via Gdb-patches [2021-06-28 13:44:26 -0400]: > This gives an internal error: > > $ ./gdb -q -nx --data-directory=data-directory -q testsuite/outputs/gdb.python/py-unwind/py-unwind -x /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-unwind.py -ex "set debug frame 1" -ex r > ... > /home/simark/src/binutils-gdb/gdb/frame.c:623: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->this_id.p != frame_id_status::COMPUTING' failed. > > The backtrace is: > > #9 0x000055d0b48e294a in internal_error (file=0x55d0b4c71380 "/home/simark/src/binutils-gdb/gdb/frame.c", line=623, fmt=0x55d0b4c712a0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:55 > #10 0x000055d0b28aade3 in get_frame_id (fi=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/frame.c:623 > #11 0x000055d0b28b5350 in get_prev_frame_id_by_id (id=) at /home/simark/src/binutils-gdb/gdb/frame.c:2601 > #12 0x000055d0b375a1ab in value_fetch_lazy_register (val=0x6110000b3500) at /home/simark/src/binutils-gdb/gdb/value.c:3955 > #13 0x000055d0b375a9e7 in value_fetch_lazy (val=0x6110000b3500) at /home/simark/src/binutils-gdb/gdb/value.c:4024 > #14 0x000055d0b28a0548 in value_of_register (regnum=6, frame=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/findvar.c:274 > #15 0x000055d0b301b05a in pending_framepy_read_register (self=0x7f79056bb7f0, args=0x7f790581aa90) at /home/simark/src/binutils-gdb/gdb/python/py-unwind.c:398 > > #27 0x000055d0b301c6b4 in pyuw_sniffer (self=0x62100018e250, this_frame=0x6210001ff5e0, cache_ptr=0x6210001ff5f8) at /home/simark/src/binutils-gdb/gdb/python/py-unwind.c:568 > #28 0x000055d0b28a6587 in frame_unwind_try_unwinder (this_frame=0x6210001ff5e0, this_cache=0x6210001ff5f8, unwinder=0x62100018e250) at /home/simark/src/binutils-gdb/gdb/frame-unwind.c:130 > #29 0x000055d0b28a67b5 in frame_unwind_find_by_frame (this_frame=0x6210001ff5e0, this_cache=0x6210001ff5f8) at /home/simark/src/binutils-gdb/gdb/frame-unwind.c:193 > #30 0x000055d0b28aa80f in compute_frame_id (fi=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/frame.c:585 > #31 0x000055d0b28aaeae in get_frame_id (fi=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/frame.c:635 > #32 0x000055d0b28b5350 in get_prev_frame_id_by_id (id=) at /home/simark/src/binutils-gdb/gdb/frame.c:2601 > #33 0x000055d0b375a1ab in value_fetch_lazy_register (val=0x6110000b3280) at /home/simark/src/binutils-gdb/gdb/value.c:3955 > #34 0x000055d0b375a9e7 in value_fetch_lazy (val=0x6110000b3280) at /home/simark/src/binutils-gdb/gdb/value.c:4024 > #35 0x000055d0b28a0548 in value_of_register (regnum=1, frame=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/findvar.c:274 > #36 0x000055d0b2826b1c in eval_op_register (expect_type=0x6210001949f0, exp=0x6030000d6840, noside=EVAL_NORMAL, name=0x60400003e2e8 "rbx") at /home/simark/src/binutils-gdb/gdb/eval.c:1074 > #37 0x000055d0b218139f in expr::register_operation::evaluate (this=0x60400003e2d0, expect_type=0x6210001949f0, exp=0x6030000d6840, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/expop.h:830 > #38 0x000055d0b282f5d5 in expr::operation::evaluate_for_cast (this=0x60400003e2d0, expect_type=0x6210001949f0, exp=0x6030000d6840, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:2506 > #39 0x000055d0b2181c64 in expr::unop_cast_operation::evaluate (this=0x6030000d6870, expect_type=0x6210001949f0, exp=0x6030000d6840, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/expop.h:1975 > #40 0x000055d0b2820b04 in expression::evaluate (this=0x6030000d6840, expect_type=0x6210001949f0, noside=EVAL_NORMAL) at /home/simark/src/binutils-gdb/gdb/eval.c:101 > #41 0x000055d0b2820c46 in evaluate_expression (exp=0x6030000d6840, expect_type=0x6210001949f0) at /home/simark/src/binutils-gdb/gdb/eval.c:115 > #42 0x000055d0b337b0fd in stap_probe::evaluate_argument (this=0x60f0000053b0, n=1, frame=0x6210001ff5e0) at /home/simark/src/binutils-gdb/gdb/stap-probe.c:1435 > #43 0x000055d0b32d86e8 in svr4_handle_solib_event () at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:1932 > #44 0x000055d0b32f1f39 in handle_solib_event () at /home/simark/src/binutils-gdb/gdb/solib.c:1258 > #45 0x000055d0b21e6c62 in bpstat_stop_status (aspace=0x6030000554e0, bp_addr=0x7ffff7fd18d8, thread=0x61700003db80, ws=0x7ffcff02c708, stop_chain=0x0) at /home/simark/src/binutils-gdb/gdb/breakpoint.c:5470 > #46 0x000055d0b2aa44c2 in handle_signal_stop (ecs=0x7ffcff02c6e0) at /home/simark/src/binutils-gdb/gdb/infrun.c:6254 > #47 0x000055d0b2aa0962 in handle_inferior_event (ecs=0x7ffcff02c6e0) at /home/simark/src/binutils-gdb/gdb/infrun.c:5740 > #48 0x000055d0b2a94a0f in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4111 > > When the get_frame_id call is done at frame 31, on frame #0, the > id is not computed yet, and therefore compute_frame_id is called. > compute_frame_id looks for an unwinder, which goes through the Python > unwinders. When value_fetch_lazy_register is called at frame 12, > get_frame_id is called again for frame #0. But we are in the process of > computing that frame id, so it can't work. The assert in get_frame_id > works as intended. > > The second call to get_frame_id happens in > > VALUE_FRAME_ID (val) > > at value.c:3955. If we dig down this, we see that: > > 1. VALUE_NEXT_FRAME_ID is called first, returning the next frame's id, > stored in struct value (it happens to be the sentinel's frame id, in > this case) > 2. it is passed to get_prev_frame_id_by_id > 3. frame_find_by_id is called on that, that returns the frame_info for > the sentinel frame > 4. get_prev_frame is called on that, that returns a frame_info for our > frame #0 > 5. get_frame_id is called on that, this is where the gdb_assert fails. > 6. had the previous step worked, value_fetch_lazy_register would have > called frame_find_by_id on it, to get the frame_info for frame #0 > > In the end, all value_fetch_lazy_register wants is a frame_info for the > value's frame. In step 4, we have that. In step 5, we fetch the > frame's id only to go back to a frame_info in step 6. That seems > unnecessary, and causes the internal error. > > Change value_fetch_lazy_register to do the more straightforward thing: > > - fetch the value's next frame id > - fetch the frame info from that > - fetch the prev frame from that (the value's frame) > > With this change, the command shown above no longer asserts and the > produced message makes sense: > > { value_fetch_lazy (frame=0,regnum=1(rbx),...) -> register=1 bytes=[c0e0fff7ff7f0000] } > > gdb/ChangeLog: > > * value.c (value_fetch_lazy_register): Don't use VALUE_FRAME_ID > to fetch value's frame for debug message. This is a duplicate of the patch I posted here: https://sourceware.org/pipermail/gdb-patches/2021-May/179361.html And then reposted here: https://sourceware.org/pipermail/gdb-patches/2021-June/180210.html In that patch I make the case that VALUE_FRAME_ID is a bad idea (due to it potentially triggering the above bug) and should be removed. I also include a test case. The only difference I see between our fixes is you went with get_prev_frame, and I went with get_prev_frame_always. I figured that if a value had a particular frame-id encoded within it then we really do want the previous frame, even if, for example, that frame is before main. Thanks, Andrew > > Change-Id: I294f06f8b32206d442da5355f1b897159414e5b6 > --- > gdb/value.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/gdb/value.c b/gdb/value.c > index 9df035a50b3b..6c969a911376 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -3883,8 +3883,6 @@ value_fetch_lazy_memory (struct value *val) > static void > value_fetch_lazy_register (struct value *val) > { > - struct frame_info *next_frame; > - int regnum; > struct type *type = check_typedef (value_type (val)); > struct value *new_val = val, *mark = value_mark (); > > @@ -3896,8 +3894,8 @@ value_fetch_lazy_register (struct value *val) > { > struct frame_id next_frame_id = VALUE_NEXT_FRAME_ID (new_val); > > - next_frame = frame_find_by_id (next_frame_id); > - regnum = VALUE_REGNUM (new_val); > + frame_info *next_frame = frame_find_by_id (next_frame_id); > + int regnum = VALUE_REGNUM (new_val); > > gdb_assert (next_frame != NULL); > > @@ -3948,18 +3946,17 @@ value_fetch_lazy_register (struct value *val) > > if (frame_debug) > { > - 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)); > - regnum = VALUE_REGNUM (val); > - gdbarch = get_frame_arch (frame); > + frame_id next_frame_id = VALUE_NEXT_FRAME_ID (val); > + frame_info *next_frame = frame_find_by_id (next_frame_id); > + frame_info *value_frame = get_prev_frame (next_frame); > + > + int regnum = VALUE_REGNUM (val); > + gdbarch *gdbarch = get_frame_arch (value_frame); > > fprintf_unfiltered (gdb_stdlog, > "{ value_fetch_lazy " > "(frame=%d,regnum=%d(%s),...) ", > - frame_relative_level (frame), regnum, > + frame_relative_level (value_frame), regnum, > user_reg_map_regnum_to_name (gdbarch, regnum)); > > fprintf_unfiltered (gdb_stdlog, "->"); > -- > 2.32.0 >