From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by sourceware.org (Postfix) with ESMTPS id 0F00E3947418 for ; Tue, 23 Jun 2020 16:34:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0F00E3947418 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-wm1-x344.google.com with SMTP id g75so3552820wme.5 for ; Tue, 23 Jun 2020 09:34:57 -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=oiTo38Zt+be3lF1SuSyxnK7PB5ZBCNVh5YyLEyneROU=; b=PvkUipM5Ha7s8kEBmPdSF0YMBkr++ApSt9oC9GNZVBA6kSbAt0TctmXh7+5By9wFkm sBUWvcXtVZ2sretw7PhJEPxv9O5sMLMczcTs3RUfjZ2Syvok1B0vYCzTJA5Xc/I0oQ74 15ABdaofax0/PI6e1DOPttB318f7QYe/h/cRzZrvoYMQOUZgDv2aCrL4rPmPbf9dUJ/s jgw4k/23fXtEbN8snrr8m0lAAkI5qCOP4TsUlXN2luKPfNSHW+ChLHCshZ/m5ZoEDWAE kbBnwcMYzoDsOKoGzdm4xfauFeZPAPWDne+HTsUAOgEMPUTRUstqe/bWxucXnG7yBuVo 7bvg== 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=oiTo38Zt+be3lF1SuSyxnK7PB5ZBCNVh5YyLEyneROU=; b=OBXLTyoeFPrZuuG7QKe8JJnHUKXcHaqdJPTdFWGNf2juSoaVDwmS50D+9KUeTsszmX UaJfVkUPdNTJJpVbYfUbj8VpwOUgTxcDbwE5DFEBqVjF2w+tsaR/ncr3699u1Ll3Uy0N otWpXSqj0dLuUdJYrvBnqNdWNlZR+QTHYTmQW32NtwM/glGvJJpXmp2WufDAFbcqU3qq iUkqiiKmx1q2KtkyPcqK3DMk6wfWpF19xygWam2GnTRLNdtv+tG9BiixbgZW7muD9pI/ 9BR2qVrEge4rpBAMOf4FhxyBjCkusfLGcsRYz4K0Oc5j5VpCC75WrU2OlTLo+OsyEddX gFdA== X-Gm-Message-State: AOAM5323e0lj9o6t8l+XisPqLSTHf+sTeYD++ORtgdXOW5Jr53KylCpx aaoO/HrkvqEFdPtoGZtYo8B2EA== X-Google-Smtp-Source: ABdhPJywKqw+75iYHjh8X0eOuyByMtshCeZmrYpAbLT/1ut4XLxoIwpSgay4sc7Bpv/yyfmUGar8BA== X-Received: by 2002:a1c:21c5:: with SMTP id h188mr20468948wmh.151.1592930095910; Tue, 23 Jun 2020 09:34:55 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id e12sm23240250wro.52.2020.06.23.09.34.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Jun 2020 09:34:55 -0700 (PDT) Date: Tue, 23 Jun 2020 17:34:54 +0100 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix "maint selftest" regression, add struct, scoped_mock_context Message-ID: <20200623163454.GP2737@embecosm.com> References: <20200414175434.8047-1-palves@redhat.com> <20200414175434.8047-29-palves@redhat.com> <20200623133750.GO2737@embecosm.com> <475a7fa3-07b1-1830-38e5-5c3b7c6cb7a1@redhat.com> <3fad7665-c2e7-5d15-151a-3adcea22f38e@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3fad7665-c2e7-5d15-151a-3adcea22f38e@redhat.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 17:34:42 up 15 days, 6:41, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, 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, 23 Jun 2020 16:34:59 -0000 * Pedro Alves [2020-06-23 16:38:02 +0100]: > On 6/23/20 3:26 PM, Pedro Alves via Gdb-patches wrote: > > On 6/23/20 2:37 PM, Andrew Burgess wrote: > > >> I suspect that the problem might be this line in regcache.c:cooked_read_test: > >> > >> /* Switch to the mock thread. */ > >> scoped_restore restore_inferior_ptid > >> = make_scoped_restore (&inferior_ptid, mock_ptid); > >> > >> My suspicion from a quick read of your patch above is that we need to > >> do more than just set inferior_ptid here now. > > > > Indeed. Thanks for reporting it. > > > > I fixed this in gdbarch-selftests.c, where this code is duplicated, > > but missed regcache.c. I'll fix it. > > > > Like so. WDYT? LGTM. Thanks, Andrew > > From d3438cc6c48bd77880da7ef7a449edbbfe990e37 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Tue, 23 Jun 2020 15:18:41 +0100 > Subject: [PATCH] Fix "maint selftest" regression, add struct > scoped_mock_context > > This commit: > > commit 3922b302645fda04da42a5279399578ae2f6206c > Author: Pedro Alves > AuthorDate: Thu Jun 18 21:28:37 2020 +0100 > > Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR 25412) > > caused a regression for gdb.gdb/unittest.exp when GDB is configured > with --enable-targets=all. The failure is: > > gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed. > > The problem is in this line in regcache.c:cooked_read_test: > > /* Switch to the mock thread. */ > scoped_restore restore_inferior_ptid > = make_scoped_restore (&inferior_ptid, mock_ptid); > > Both gdbarch-selftest.c and regcache.c set up a similar mock context, > but the series the patch above belongs to only updated the > gdbarch-selftest.c context to not write to inferior_ptid directly, and > missed updating regcache.c's. > > Instead of copying the fix over to regcache.c, share the mock context > setup code in a new RAII class, based on gdbarch-selftest.c's version. > > Also remove the "target already pushed" error from regcache.c, like it > had been removed from gdbarch-selftest.c in the multi-target series. > That check is unnecessary because each inferior now has its own target > stack, and the unit test pushes a target on a separate (mock) > inferior, not the current inferior on entry. > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > * gdbarch-selftests.c: Don't include inferior.h, gdbthread.h or > progspace-and-thread.h. Include scoped-mock-context.h instead. > (register_to_value_test): Use scoped_mock_context. > * regcache.c: Include "scoped-mock-context.h". > (cooked_read_test): Dont error out if a target is already pushed. > Use scoped_mock_context. Adjust. > * scoped-mock-context.h: New file. > --- > gdb/gdbarch-selftests.c | 39 ++-------------------- > gdb/regcache.c | 71 +++++++++------------------------------- > gdb/scoped-mock-context.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 99 insertions(+), 93 deletions(-) > create mode 100644 gdb/scoped-mock-context.h > > diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c > index 91aa9d87344..4f9adbd9e9c 100644 > --- a/gdb/gdbarch-selftests.c > +++ b/gdb/gdbarch-selftests.c > @@ -20,14 +20,12 @@ > #include "defs.h" > #include "gdbsupport/selftest.h" > #include "selftest-arch.h" > -#include "inferior.h" > -#include "gdbthread.h" > #include "target.h" > #include "test-target.h" > #include "target-float.h" > #include "gdbsupport/def-vector.h" > #include "gdbarch.h" > -#include "progspace-and-thread.h" > +#include "scoped-mock-context.h" > > namespace selftests { > > @@ -71,40 +69,7 @@ register_to_value_test (struct gdbarch *gdbarch) > builtin->builtin_char32, > }; > > - /* Create a mock environment. An inferior with a thread, with a > - process_stratum target pushed. */ > - > - test_target_ops mock_target; > - ptid_t mock_ptid (1, 1); > - program_space mock_pspace (new_address_space ()); > - inferior mock_inferior (mock_ptid.pid ()); > - mock_inferior.gdbarch = gdbarch; > - mock_inferior.aspace = mock_pspace.aspace; > - mock_inferior.pspace = &mock_pspace; > - thread_info mock_thread (&mock_inferior, mock_ptid); > - > - scoped_restore_current_pspace_and_thread restore_pspace_thread; > - > - scoped_restore restore_thread_list > - = make_scoped_restore (&mock_inferior.thread_list, &mock_thread); > - > - /* Add the mock inferior to the inferior list so that look ups by > - target+ptid can find it. */ > - scoped_restore restore_inferior_list > - = make_scoped_restore (&inferior_list, &mock_inferior); > - > - /* Switch to the mock inferior. */ > - switch_to_inferior_no_thread (&mock_inferior); > - > - /* Push the process_stratum target so we can mock accessing > - registers. */ > - push_target (&mock_target); > - > - /* Pop it again on exit (return/exception). */ > - SCOPE_EXIT { pop_all_targets_at_and_above (process_stratum); }; > - > - /* Switch to the mock thread. */ > - switch_to_thread (&mock_thread); > + scoped_mock_context mockctx (gdbarch); > > struct frame_info *frame = get_current_frame (); > const int num_regs = gdbarch_num_cooked_regs (gdbarch); > diff --git a/gdb/regcache.c b/gdb/regcache.c > index 6a4359d0f36..4ebb8cb0452 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -22,6 +22,7 @@ > #include "gdbthread.h" > #include "target.h" > #include "test-target.h" > +#include "scoped-mock-context.h" > #include "gdbarch.h" > #include "gdbcmd.h" > #include "regcache.h" > @@ -1596,49 +1597,7 @@ class readwrite_regcache : public regcache > static void > cooked_read_test (struct gdbarch *gdbarch) > { > - /* Error out if debugging something, because we're going to push the > - test target, which would pop any existing target. */ > - if (current_top_target ()->stratum () >= process_stratum) > - error (_("target already pushed")); > - > - /* Create a mock environment. An inferior with a thread, with a > - process_stratum target pushed. */ > - > - target_ops_no_register mock_target; > - ptid_t mock_ptid (1, 1); > - inferior mock_inferior (mock_ptid.pid ()); > - address_space mock_aspace {}; > - mock_inferior.gdbarch = gdbarch; > - mock_inferior.aspace = &mock_aspace; > - thread_info mock_thread (&mock_inferior, mock_ptid); > - mock_inferior.thread_list = &mock_thread; > - > - /* Add the mock inferior to the inferior list so that look ups by > - target+ptid can find it. */ > - scoped_restore restore_inferior_list > - = make_scoped_restore (&inferior_list); > - inferior_list = &mock_inferior; > - > - /* Switch to the mock inferior. */ > - scoped_restore_current_inferior restore_current_inferior; > - set_current_inferior (&mock_inferior); > - > - /* Push the process_stratum target so we can mock accessing > - registers. */ > - push_target (&mock_target); > - > - /* Pop it again on exit (return/exception). */ > - struct on_exit > - { > - ~on_exit () > - { > - pop_all_targets_at_and_above (process_stratum); > - } > - } pop_targets; > - > - /* Switch to the mock thread. */ > - scoped_restore restore_inferior_ptid > - = make_scoped_restore (&inferior_ptid, mock_ptid); > + scoped_mock_context mockctx (gdbarch); > > /* Test that read one raw register from regcache_no_target will go > to the target layer. */ > @@ -1653,21 +1612,21 @@ cooked_read_test (struct gdbarch *gdbarch) > break; > } > > - readwrite_regcache readwrite (&mock_target, gdbarch); > + readwrite_regcache readwrite (&mockctx.mock_target, gdbarch); > gdb::def_vector buf (register_size (gdbarch, nonzero_regnum)); > > readwrite.raw_read (nonzero_regnum, buf.data ()); > > /* raw_read calls target_fetch_registers. */ > - SELF_CHECK (mock_target.fetch_registers_called > 0); > - mock_target.reset (); > + SELF_CHECK (mockctx.mock_target.fetch_registers_called > 0); > + mockctx.mock_target.reset (); > > /* Mark all raw registers valid, so the following raw registers > accesses won't go to target. */ > for (auto i = 0; i < gdbarch_num_regs (gdbarch); i++) > readwrite.raw_update (i); > > - mock_target.reset (); > + mockctx.mock_target.reset (); > /* Then, read all raw and pseudo registers, and don't expect calling > to_{fetch,store}_registers. */ > for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++) > @@ -1680,18 +1639,18 @@ cooked_read_test (struct gdbarch *gdbarch) > SELF_CHECK (REG_VALID == readwrite.cooked_read (regnum, > inner_buf.data ())); > > - SELF_CHECK (mock_target.fetch_registers_called == 0); > - SELF_CHECK (mock_target.store_registers_called == 0); > - SELF_CHECK (mock_target.xfer_partial_called == 0); > + SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0); > + SELF_CHECK (mockctx.mock_target.store_registers_called == 0); > + SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0); > > - mock_target.reset (); > + mockctx.mock_target.reset (); > } > > readonly_detached_regcache readonly (readwrite); > > /* GDB may go to target layer to fetch all registers and memory for > readonly regcache. */ > - mock_target.reset (); > + mockctx.mock_target.reset (); > > for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++) > { > @@ -1749,11 +1708,11 @@ cooked_read_test (struct gdbarch *gdbarch) > } > } > > - SELF_CHECK (mock_target.fetch_registers_called == 0); > - SELF_CHECK (mock_target.store_registers_called == 0); > - SELF_CHECK (mock_target.xfer_partial_called == 0); > + SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0); > + SELF_CHECK (mockctx.mock_target.store_registers_called == 0); > + SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0); > > - mock_target.reset (); > + mockctx.mock_target.reset (); > } > } > > diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h > new file mode 100644 > index 00000000000..461c2a35388 > --- /dev/null > +++ b/gdb/scoped-mock-context.h > @@ -0,0 +1,82 @@ > +/* RAII type to create a temporary mock context. > + > + Copyright (C) 2020 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#ifndef SCOPED_MOCK_CONTEXT_H > +#define SCOPED_MOCK_CONTEXT_H > + > +#include "inferior.h" > +#include "gdbthread.h" > +#include "progspace.h" > +#include "progspace-and-thread.h" > + > +#if GDB_SELF_TEST > +namespace selftests { > + > +/* RAII type to create (and switch to) a temporary mock context. An > + inferior with a thread, with a process_stratum target pushed. */ > + > +template > +struct scoped_mock_context > +{ > + /* Order here is important. */ > + > + Target mock_target; > + ptid_t mock_ptid {1, 1}; > + program_space mock_pspace {new_address_space ()}; > + inferior mock_inferior {mock_ptid.pid ()}; > + thread_info mock_thread {&mock_inferior, mock_ptid}; > + > + scoped_restore_current_pspace_and_thread restore_pspace_thread; > + > + scoped_restore_tmpl restore_thread_list > + {&mock_inferior.thread_list, &mock_thread}; > + > + /* Add the mock inferior to the inferior list so that look ups by > + target+ptid can find it. */ > + scoped_restore_tmpl restore_inferior_list > + {&inferior_list, &mock_inferior}; > + > + explicit scoped_mock_context (gdbarch *gdbarch) > + { > + mock_inferior.gdbarch = gdbarch; > + mock_inferior.aspace = mock_pspace.aspace; > + mock_inferior.pspace = &mock_pspace; > + > + /* Switch to the mock inferior. */ > + switch_to_inferior_no_thread (&mock_inferior); > + > + /* Push the process_stratum target so we can mock accessing > + registers. */ > + gdb_assert (mock_target.stratum () == process_stratum); > + push_target (&mock_target); > + > + /* Switch to the mock thread. */ > + switch_to_thread (&mock_thread); > + } > + > + ~scoped_mock_context () > + { > + pop_all_targets_at_and_above (process_stratum); > + } > +}; > + > +} // namespace selftests > +#endif /* GDB_SELF_TEST */ > + > +#endif /* !defined (SCOPED_MOCK_CONTEXT_H) */ > > base-commit: 39ff0b812324f4b050bb0b367b269db6d4d0cb8b > -- > 2.14.5 >