Re: [PATCH 1/3] Redesign mock environment for gdbarch selftests

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

> If there's already a running target when you type "maint selftest",
> then we bail out, instead of pushing a new target on top of the
> existing one (and thus killing the debug session).  I think that's OK,
> because self tests are really mean to be run from a clean state right
> after GDB is started.  I'm adding that skipping just as safe measure
> just in case someone (as I've done it) types "maint selftest" on the
> command line while already debugging something.

That is a goo catch, but IMO, we shouldn't allow running unit tests
during debugging session.  GDB unit tests touch many things, and some of
them are global states in GDB.  It is difficult to know what global
states we need to restore after tests.

>
> (In my multi-target branch, where this patch originated from, we don't
> actually need to bail out, because there each inferior has its own
> target stack).
>
> Also, note that the current code was doing:
>
>  current_inferior()->gdbarch = gdbarch;
>
> without taking care to restore the previous gdbarch.  This means that
> GDB's state was being left inconsistent after running the self tests,,
> further supporting the point that there's probably not much
> expectation that mixing "maint selftests" and regular debugging in the
> same GDB invocation really works.

You are right, we should manage the expectation of mixing debugging and
"maint selftests".  My suggestion is that "please do run selftests
with regular debugging".

> -    raw_set_cached_value (regnum, buf);
> +    to_magic = OPS_MAGIC;
> +    to_stratum = process_stratum;
> +    to_has_memory = test_target_has_memory;
> +    to_has_stack = test_target_has_stack;
> +    to_has_registers = test_target_has_registers;
> +    to_prepare_to_store = test_target_prepare_to_store;
> +    to_fetch_registers = test_target_fetch_registers;

to_fetch_registers is TARGET_DEFAULT_IGNORE, so we don't need define
test_target_fetch_registers if we disallow running selftests with
regular debugging.

> +    to_store_registers = test_target_store_registers;

> +    to_thread_architecture = default_thread_architecture;
> +    to_thread_address_space = default_thread_address_space;

Any reason to set to_thread_address_space and to_thread_architecture?
Can't we use the default value?

-- 
Yao (齐尧)