Re: [PATCH] Fix "maint selftest" regression, add struct, scoped_mock_context

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [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 <palves@redhat.com>
> 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 <palves@redhat.com>
>  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  <palves@redhat.com>
> 
> 	* 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<test_target_ops> 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<target_ops_no_register> 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<gdb_byte> 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 <http://www.gnu.org/licenses/>.  */
> +
> +#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<typename Target>
> +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<thread_info *> 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<inferior *> 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
>