From patchwork Tue Oct 3 12:05:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 23292 Received: (qmail 25670 invoked by alias); 3 Oct 2017 12:05:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 25661 invoked by uid 89); 3 Oct 2017 12:05:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Running, strive X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Oct 2017 12:05:41 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B2B5776D8; Tue, 3 Oct 2017 12:05:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5B2B5776D8 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6713E5C1A1; Tue, 3 Oct 2017 12:05:39 +0000 (UTC) Subject: Re: [PATCH 1/3] Redesign mock environment for gdbarch selftests To: Yao Qi References: <1506957311-30028-1-git-send-email-palves@redhat.com> <1506957311-30028-2-git-send-email-palves@redhat.com> <86k20c7aob.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Tue, 3 Oct 2017 13:05:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <86k20c7aob.fsf@gmail.com> On 10/03/2017 12:06 PM, Yao Qi wrote: > Pedro Alves 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. On the restore side, I think we should treat those as bugs to look for when reviewing/working on unit tests, because: #1 - it's convenient for development to be able to run unit tests multiple times in the same session. #2 - because we should strive to avoid one unit test influencing the environment of following unrelated unit tests. On the "allow selftests during debugging", I think it's reasonable to bail out if/when it'd be very difficult otherwise. In this case, maybe I could get it working even while there's already a target by save/restoring current_target too. I didn't bother with that because with multi-target, the problem solves itself simply because the current_target global disappears and it felt like unnecessary complication. > >> >> (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". OK. I propose we stick with something like I had, and still build an isolated inferior/aspace etc. To me, that feels like more robust mocking/isolation. If I run the tests while debugging something with the updated patch (below), I get: (gdb) maint selftest Running selftest array_view. Running selftest copy_bitwise. Running selftest copy_integer_to_size. Running selftest current_regcache. Running selftest execute_cfa_program. Running selftest function_view. Running selftest gdb_environ. Running selftest gdb_realpath. Running selftest memory_error. Running selftest offset_type. Running selftest optional. Running selftest print_one_insn. Running selftest producer-parser. Running selftest register_to_value. Self test failed: arch i386: target already pushed Self test failed: arch i386:x86-64: target already pushed Self test failed: arch i386:x64-32: target already pushed Self test failed: arch i8086: target already pushed Self test failed: arch i386:intel: target already pushed Self test failed: arch i386:x86-64:intel: target already pushed Self test failed: arch i386:x64-32:intel: target already pushed Self test failed: arch i386:nacl: target already pushed Self test failed: arch i386:x86-64:nacl: target already pushed Self test failed: arch i386:x64-32:nacl: target already pushed Self test failed: self-test failed at /home/pedro/gdb/mygit/src/gdb/selftest-arch.c:86 Running selftest rust-lex. Running selftest scoped_restore. Running selftest string_printf. Running selftest string_vprintf. Running selftest xml_escape_text. Ran 19 unit tests, 1 failed So it ends up being best effort, with most things passing and the gdbarch test failing. WDYT? (I've switched to checking for current_target.to_stratum >= process_stratum instead of target_has_execution, as it's a more accurate test. target_has_execution would return false when debugging a core, or when connected to an extended-remote target before typing run, to name a couple examples.) > > 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. I've made that change. (the reason I defined it is not related to regular debugging. see below.) > >> + 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? > The reason is that in the multi-target branch target_ops is a real C++ class hierarchy and is different enough in details that I forgot how things worked in current master. :-P The problem is that I forgot that we need to call complete_target_initialization, in order to install the defaults in the target, so without the explicit "= default_thread_...;", I was ending up with those methods set to NULL, and GDB would crash... Here's the updated patch. From d2287dfe6bafaa8a3bfd3720fd660a1aacf4693a Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 3 Oct 2017 12:24:08 +0100 Subject: [PATCH] Redesign mock environment for gdbarch selftests A following patch will remove this hack from within regcache's implementation: struct regcache * get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch) { struct address_space *aspace; /* For the benefit of "maint print registers" & co when debugging an executable, allow dumping the regcache even when there is no thread selected (target_thread_address_space internal-errors if no address space is found). Note that normal user commands will fail higher up on the call stack due to no target_has_registers. */ aspace = (ptid_equal (null_ptid, ptid) ? NULL : target_thread_address_space (ptid)); i.e., it'll no longer be possible to try to build a regcache for null_ptid. That change alone would regress the gdbarch self tests though, causing this: (gdb) maintenance selftest [...] Running selftest register_to_value. src/gdb/inferior.c:309: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.gdb/unittest.exp: maintenance selftest (GDB internal error) The problem is that the way the mocking environment for those unit tests is written is a bit fragile: it creates a special purpose regcache (and sentinel's frame), using whatever is the current inferior_ptid (usually null_ptid), and assumes get_current_regcache will find that in the regcache::current_regcache list. This commit changes the way the mock environment is created. It eliminates the special regcache and frame and instead creates a fuller mock environment, with a custom mock target_ops, and then a mock inferior and thread "running" on that target. If there's already a running target when you type "maint selftest", then we error out, instead of pushing a new target on top of the existing one (and thus killing the debug session). This results in: (gdb) maint selftest (...) Self test failed: arch i386: target already pushed Self test failed: arch i386:x86-64: target already pushed Self test failed: arch i386:x64-32: target already pushed Self test failed: arch i8086: target already pushed Self test failed: arch i386:intel: target already pushed Self test failed: arch i386:x86-64:intel: target already pushed Self test failed: arch i386:x64-32:intel: target already pushed Self test failed: arch i386:nacl: target already pushed Self test failed: arch i386:x86-64:nacl: target already pushed Self test failed: arch i386:x64-32:nacl: target already pushed Self test failed: self-test failed at /home/pedro/gdb/mygit/src/gdb/selftest-arch.c:86 (...) Ran 19 unit tests, 1 failed I think that's OK, because self tests are really meant to be run from a clean state right after GDB is started. I'm adding that erroring out just as safe measure just in case someone types "maint selftest" on the command line while already debugging something (as I've done it). (In my multi-target branch, where this patch originated from, we don't actually need to error 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. This patch fixes that, regardless. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * frame.c (create_test_frame): Delete. * frame.h (create_test_frame): Delete. * gdbarch-selftests.c: Include gdbthread.h and target.h. (class regcache_test): Delete. (test_target_has_registers, test_target_has_stack) (test_target_has_memory, test_target_prepare_to_store) (test_target_store_registers): New functions. (test_target_ops): New class. (register_to_value_test): Error out if there's already a process_stratum (or higher) target pushed. Create a fuller mock environment, with mock target_ops, inferior, address space, thread and inferior_ptid. * progspace.c (struct address_space): Move to ... * progspace.h (struct address_space): ... here. * regcache.h (regcache::~regcache, regcache::raw_write) [GDB_SELF_TEST]: No longer virtual. --- gdb/frame.c | 17 -------- gdb/frame.h | 8 ---- gdb/gdbarch-selftests.c | 105 ++++++++++++++++++++++++++++++++++++++++-------- gdb/progspace.c | 12 ------ gdb/progspace.h | 11 +++++ gdb/regcache.h | 10 ----- 6 files changed, 100 insertions(+), 63 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index a74deef..afdc157 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1716,23 +1716,6 @@ select_frame (struct frame_info *fi) } } -#if GDB_SELF_TEST -struct frame_info * -create_test_frame (struct regcache *regcache) -{ - struct frame_info *this_frame = XCNEW (struct frame_info); - - sentinel_frame = create_sentinel_frame (NULL, regcache); - sentinel_frame->prev = this_frame; - sentinel_frame->prev_p = 1;; - this_frame->prev_arch.p = 1; - this_frame->prev_arch.arch = get_regcache_arch (regcache); - this_frame->next = sentinel_frame; - - return this_frame; -} -#endif - /* Create an arbitrary (i.e. address specified by user) or innermost frame. Always returns a non-NULL value. */ diff --git a/gdb/frame.h b/gdb/frame.h index 0249857..1b72610 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -833,14 +833,6 @@ extern struct frame_info *deprecated_safe_get_selected_frame (void); extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc); -#if GDB_SELF_TEST - -/* Create a frame for unit test. Its next frame is sentinel frame, - created from REGCACHE. */ - -extern struct frame_info *create_test_frame (struct regcache *regcache); -#endif - /* Return true if the frame unwinder for frame FI is UNWINDER; false otherwise. */ diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c index f0b8d5d..58ed112 100644 --- a/gdb/gdbarch-selftests.c +++ b/gdb/gdbarch-selftests.c @@ -22,25 +22,57 @@ #include "selftest.h" #include "selftest-arch.h" #include "inferior.h" +#include "gdbthread.h" +#include "target.h" namespace selftests { -/* A read-write regcache which doesn't write the target. */ +/* A mock process_stratum target_ops that doesn't read/write registers + anywhere. */ -class regcache_test : public regcache +static int +test_target_has_registers (target_ops *self) { -public: - explicit regcache_test (struct gdbarch *gdbarch) - : regcache (gdbarch, NULL, false) - { - set_ptid (inferior_ptid); + return 1; +} - current_regcache.push_front (this); - } +static int +test_target_has_stack (target_ops *self) +{ + return 1; +} + +static int +test_target_has_memory (target_ops *self) +{ + return 1; +} + +static void +test_target_prepare_to_store (target_ops *self, regcache *regs) +{ +} + +static void +test_target_store_registers (target_ops *self, regcache *regs, int regno) +{ +} - void raw_write (int regnum, const gdb_byte *buf) override +class test_target_ops : public target_ops +{ +public: + test_target_ops () + : target_ops {} { - 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_store_registers = test_target_store_registers; + + complete_target_initialization (this); } }; @@ -84,15 +116,56 @@ register_to_value_test (struct gdbarch *gdbarch) builtin->builtin_char32, }; - current_inferior()->gdbarch = gdbarch; + /* Error out if debugging something, because we're going to push the + test target, which would pop any existing target. */ + if (current_target.to_stratum >= process_stratum) + error (_("target already pushed")); + + /* 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); + 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); + + scoped_restore restore_thread_list + = make_scoped_restore (&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); - struct regcache *regcache = new regcache_test (gdbarch); - struct frame_info *frame = create_test_frame (regcache); + struct frame_info *frame = get_current_frame (); const int num_regs = (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch)); - SELF_CHECK (regcache == get_current_regcache ()); - /* Test gdbarch methods register_to_value and value_to_register with different combinations of register numbers and types. */ for (const auto &type : types) diff --git a/gdb/progspace.c b/gdb/progspace.c index 41e8cd0..ea328c8 100644 --- a/gdb/progspace.c +++ b/gdb/progspace.c @@ -44,18 +44,6 @@ static int highest_address_space_num; DEFINE_REGISTRY (program_space, REGISTRY_ACCESS_FIELD) -/* An address space. It is used for comparing if pspaces/inferior/threads - see the same address space and for associating caches to each address - space. */ - -struct address_space -{ - int num; - - /* Per aspace data-pointers required by other GDB modules. */ - REGISTRY_FIELDS; -}; - /* Keep a registry of per-address_space data-pointers required by other GDB modules. */ diff --git a/gdb/progspace.h b/gdb/progspace.h index f679fe3..85c99a62 100644 --- a/gdb/progspace.h +++ b/gdb/progspace.h @@ -210,6 +210,17 @@ struct program_space REGISTRY_FIELDS; }; +/* An address space. It is used for comparing if + pspaces/inferior/threads see the same address space and for + associating caches to each address space. */ +struct address_space +{ + int num; + + /* Per aspace data-pointers required by other GDB modules. */ + REGISTRY_FIELDS; +}; + /* The object file that the main symbol table was loaded from (e.g. the argument to the "symbol-file" or "file" command). */ diff --git a/gdb/regcache.h b/gdb/regcache.h index 3ecdb3b..6f42fb93 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -252,11 +252,6 @@ public: DISABLE_COPY_AND_ASSIGN (regcache); - /* class regcache is only extended in unit test, so only mark it - virtual when selftest is enabled. */ -#if GDB_SELF_TEST - virtual -#endif ~regcache () { xfree (m_registers); @@ -277,11 +272,6 @@ public: enum register_status raw_read (int regnum, gdb_byte *buf); - /* class regcache is only extended in unit test, so only mark it - virtual when selftest is enabled. */ -#if GDB_SELF_TEST - virtual -#endif void raw_write (int regnum, const gdb_byte *buf); template>