From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x343.google.com (mail-wm1-x343.google.com [IPv6:2a00:1450:4864:20::343]) by sourceware.org (Postfix) with ESMTPS id EEE803947C3A for ; Tue, 23 Jun 2020 13:37:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EEE803947C3A 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-x343.google.com with SMTP id 17so3262211wmo.1 for ; Tue, 23 Jun 2020 06:37:53 -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=NBheGkwRsVE+txuRNgAMRp1YE21K89hjkopuLzcP4/8=; b=GY2qH9LlyfvqHoDyd0ee5ZRVPso7oe2GV1svTuqbUVg5NffyMfgGunryDJ5fdQ2ndV TPtPorCOAjHitX2AGisZTH6BAHh3geK5PKphsZAPh+/jwAH/aLDvHkuW4CFee0I1Alke CSQFRRdVyHeop+LN7xph5xo6+Y4HHjhKGGIQ4dK9SVXmxColJNW47YhVLeKFR6rra/4D 7JCZWSYk9/cM4XjfoQQw2JbFBHevYxkNMS/B8gCnTrd1AkwPGb3WhJGE+p7wBUgmS74i wlLyvUT/6tIp2sV9gIm9HM4AEZ84Tvd0PoiCZ0TecgNscJ2Q2RRpUDdhzh0zYnCmddyO 6vJg== 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=NBheGkwRsVE+txuRNgAMRp1YE21K89hjkopuLzcP4/8=; b=SC5iT9tT816pzAmg4JXoULP5XvF5UsYBI6E5lR/QJZGrwYputVEhuOxu9VlF4aNlLo ZG+wh7CO1X0Gf7qni751lPb/rOdPTBoiWq+tEFgcFGDiCdBAEgCUyxX52cLObbH9gU2H 2pnP5ZViz4zSOgYA/FVJz8Y2B2hFoZtdst/T23mB28VSOmCnEfV3KM/x80yTmlQhIBGI z5YILCxwoq9xhGjjziYPCa/DrvRRrwsluMO7TCVMdN5ZHq3JfYXfc8eahFbyivOQQ19J CPKLegPpt8rtMBMoasFJMSPicTVhzek0V0CQziGh9Z2M4UbNhlJo65k+lSItU18QTP6+ U7uA== X-Gm-Message-State: AOAM531k6GVYEkDoQELD1nzD4XO1EOHKzY23dcH13w4y+rDkidCLfIqL 0F/OayPLmAmCKg1gBmlKFXGVFo5NYEo= X-Google-Smtp-Source: ABdhPJwPZ1tI5IyH98CjWP/ik6Bv8FTB8zZkJHawQ7IrRHSmLPf/IcghhAYTSDouNmXfFm2UA71j9w== X-Received: by 2002:a7b:cd83:: with SMTP id y3mr23793846wmj.5.1592919472639; Tue, 23 Jun 2020 06:37:52 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id d13sm11149291wrq.89.2020.06.23.06.37.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Jun 2020 06:37:51 -0700 (PDT) Date: Tue, 23 Jun 2020 14:37:50 +0100 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412) Message-ID: <20200623133750.GO2737@embecosm.com> References: <20200414175434.8047-1-palves@redhat.com> <20200414175434.8047-29-palves@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200414175434.8047-29-palves@redhat.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 14:29:53 up 15 days, 3:36, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-1.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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 13:37:56 -0000 * Pedro Alves via Gdb-patches [2020-04-14 18:54:34 +0100]: > In PR/25412, Simon noticed that after the multi-target series, the > tid-reuse.exp testcase manages to create a duplicate thread in the > thread list. Or rather, two threads with the same PTID. > > add_thread_silent has code in place to detect the case of a new thread > reusing some older thread's ptid, but it doesn't work correctly > anymore when the old thread is NOT the current thread and it has a > refcount higher than 0. Either condition prevents a thread from being > deleted, but the refcount case wasn't being considered. I think the > reason that case wasn't considered is that that code predates > thread_info refcounting. Back when it was originally written, > delete_thread always deleted the thread. > > That add_thread_silent code in question has some now-unnecessary > warts, BTW. For instance, this: > > /* Make switch_to_thread not read from the thread. */ > new_thr->state = THREAD_EXITED; > > ... used to be required because switch_to_thread would update > 'stop_pc' otherwise. I.e., it would read registers from an exited > thread otherwise. switch_to_thread no longer reads the stop_pc, since: > > commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09 > Author: Pedro Alves > AuthorDate: Thu Jun 28 20:18:24 2018 +0100 > > gdb: Eliminate the 'stop_pc' global > > Also, if the ptid of the now-gone current thread is reused, we > currently return from add_thread_silent with the current thread > pointing at the _new_ thread. Either pointing at the old thread, or > at no thread selected would be reasonable. But pointing at an > unrelated thread (the new thread that happens to reuse the ptid) is > just broken. Seems like I was the one who wrote it like that but I > have no clue why, FWIW. > > Currently, an exited thread kept in the thread list still holds its > original ptid. The idea was that we need the ptid to be able to > temporarily switch to another thread and then switch back to the > original thread, because thread switching is really inferior_ptid > switching. Switching back to the original thread requires a ptid > lookup. > > Now, in order to avoid exited threads with the same ptid as a live > thread in the same thread list, one thing I considered (and tried) was > to change an exited thread's ptid to minus_one_ptid. However, with > that, there's a case that we won't handle well, which is if we end up > with more than one exited thread in the list, since then all exited > threads will all have the same ptid. Since inferior_thread() relies > on inferior_ptid, may well return the wrong thread. > > My next attempt to address this, was to switch an exited thread's ptid > to a globally unique "exited" ptid, which is a ptid with pid == -1 and > tid == 'the thread's global GDB thread number'. Note that GDB assumes > that the GDB global thread number is monotonically increasing and > doesn't wrap around. (We should probably make GDB thread numbers > 64-bit to prevent that happening in practice; they're currently signed > 32-bit.) This attempt went a long way, but still ran into a number of > issues. It was a major hack too, obviously. > > My next attempt is the one that I'm proposing, which is to bite the > bullet and break the connection between inferior_ptid and > inferior_thread(), aka the current thread. I.e., make the current > thread be a global thread_info pointer that is written to directly by > switch_to_thread, etc., and making inferior_thread() return that > pointer, instead of having inferior_thread() lookup up the > inferior_ptid thread, by ptid_t. You can look at this as a > continuation of the effort of using more thread_info pointers instead > of ptids when possible. > > By making the current thread a global thread_info pointer, we can make > switch_to_thread simply write to the global thread pointer, which > makes scoped_restore_current_thread able to restore back to an exited > thread without relying on unrelyable ptid look ups. I.e., this makes > it not a real problem to have more than one thread with the same ptid > in the thread list. There will always be only one live thread with a > given ptid, so code that looks up a live thread by ptid will always be > able to find the right one. > > This change required auditing the whole codebase for places where we > were writing to inferior_ptid directly to change the current thread, > and change them to use switch_to_thread instead or one of its > siblings, because otherwise inferior_thread() would return a thread > unrelated to the changed-to inferior_ptid. That was all (hopefully) > done in previous patches. > > After this, inferior_ptid is mainly used by target backend code. It > is also relied on by a number of target methods. E.g., the > target_resume interface and the memory reading routines -- we still > need it there because we need to be able to access memory off of > processes for which we don't have a corresponding inferior/thread > object, like when handling forks. Maybe we could pass down a context > explicitly to target_read_memory, etc. > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > * gdbthread.h (delete_thread, delete_thread_silent) > (find_thread_ptid): Update comments. > * thread.c (current_thread_): New global. > (is_current_thread): Move higher, and reimplement. > (inferior_thread): Reimplement. > (set_thread_exited): Use bool. Add assertions. > (add_thread_silent): Simplify thread-reuse handling by always > calling delete_thread. > (delete_thread): Remove intro comment. > (find_thread_ptid): Skip exited threads. > (switch_to_thread_no_regs): Write to current_thread_. > (switch_to_no_thread): Check CURRENT_THREAD_ instead of > INFERIOR_PTID. Clear current_thread_. > --- > gdb/gdbthread.h | 17 +++++----- > gdb/thread.c | 97 ++++++++++++++++++++------------------------------------- > 2 files changed, 42 insertions(+), 72 deletions(-) This commit causes 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. Here's a partial backtrace: #8 0x00000000013e4d0f in internal_error (file=0x16c55c7 "../../src.dev-1/gdb/thread.c", line=95, fmt=0x16c54d9 "%s: Assertion `%s' failed.") at ../../src.dev-1/gdbsupport/errors.cc:55 #9 0x0000000000c34e63 in inferior_thread () at ../../src.dev-1/gdb/thread.c:95 #10 0x0000000000a73d62 in get_current_regcache () at ../../src.dev-1/gdb/regcache.c:391 #11 0x00000000009051c0 in current_options () at ../../src.dev-1/gdb/mep-tdep.c:873 #12 0x00000000009052eb in mep_register_name (gdbarch=0x33862a0, regnr=152) at ../../src.dev-1/gdb/mep-tdep.c:958 #13 0x00000000009054aa in mep_register_reggroup_p (gdbarch=0x33862a0, regnum=152, group=0x26334c0 ) at ../../src.dev-1/gdb/mep-tdep.c:1029 #14 0x00000000007975e8 in gdbarch_register_reggroup_p (gdbarch=0x33862a0, regnum=152, reggroup=0x26334c0 ) at ../../src.dev-1/gdb/gdbarch.c:3595 #15 0x0000000000a7355a in reg_buffer::save(gdb::function_view) ( this=0x7fffffffad00, cooked_read=...) at ../../src.dev-1/gdb/regcache.c:248 #16 0x0000000000780b36 in readonly_detached_regcache::readonly_detached_regcache(gdbarch*, gdb::function_view) (this=0x7fffffffad00, gdbarch=0x33862a0, cooked_read=...) at ../../src.dev-1/gdb/regcache.h:455 #17 0x0000000000a73409 in readonly_detached_regcache::readonly_detached_regcache (this=0x7fffffffad00, src=...) at ../../src.dev-1/gdb/regcache.c:213 #18 0x0000000000a773b5 in selftests::cooked_read_test (gdbarch=0x33862a0) at ../../src.dev-1/gdb/regcache.c:1690 #19 0x0000000000b48c29 in selftests::gdbarch_selftest::operator() (this=0x320fef0) at ../../src.dev-1/gdb/selftest-arch.c:73 #20 0x00000000013fe5ac in selftests::run_tests (filter=0x0) at ../../src.dev-1/gdbsupport/selftest.cc:88 #21 0x00000000008e78ba in maintenance_selftest (args=0x0, from_tty=1) at ../../src.dev-1/gdb/maint.c:1044 #22 0x00000000005b0557 in do_const_cfunc (c=0x32004f0, args=0x0, from_tty=1) at ../../src.dev-1/gdb/cli/cli-decode.c:95 #23 0x00000000005b3839 in cmd_func (cmd=0x32004f0, args=0x0, from_tty=1) at ../../src.dev-1/gdb/cli/cli-decode.c:2113 #24 0x0000000000c4953b in execute_command (p=0x2924895 "", from_tty=1) at ../../src.dev-1/gdb/top.c:655 #25 0x0000000000759c26 in command_handler (command=0x2924880 "maintenance selftest ") at ../../src.dev-1/gdb/event-top.c:588 #26 0x000000000075a052 in command_line_handler (rl=...) at ../../src.dev-1/gdb/event-top.c:773 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. Thanks, Andrew