From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Mw96IJrBqGDGEgAAWB0awg (envelope-from ) for ; Sat, 22 May 2021 04:32:26 -0400 Received: by simark.ca (Postfix, from userid 112) id 7722F1F11C; Sat, 22 May 2021 04:32:26 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 7BB891E54D for ; Sat, 22 May 2021 04:32:24 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BB363383B417; Sat, 22 May 2021 08:32:23 +0000 (GMT) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 1DD0D3857819 for ; Sat, 22 May 2021 08:32:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1DD0D3857819 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1621672339; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ofLlzvqI2m5blAHy0b5NP1N6o9BSjdxp3dtLYnI7LtA=; b=l6ByI4pjJp+USAcA0UF4kzjiZgcqzahDeVkkeTBOtN3EoYmStS9pVVYAKWNAD8Sr8FtMWN p07sM8YnokJwugrnjrNRJ7S1qTpwcMCWMImHmFl3Qv4ul870JxjM8pPTWIIRb7siHrR6OV tVOIOUAtyK2CYUbOeOKroXtfVvEtLeY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1621672339; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ofLlzvqI2m5blAHy0b5NP1N6o9BSjdxp3dtLYnI7LtA=; b=0Jln9waWsUPs3Zrn1VRKhjvFvbTUy74NfTrfv45VRBZOJhunv9sjdKtkspvU1Tm5DAjgaZ FKxy3vMNvThFtiDQ== Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 23B76ABC2; Sat, 22 May 2021 08:32:19 +0000 (UTC) Subject: Re: [PATCH][gdb/tdep] Use pid to choose x86_64 process 64/32-bitness To: Alan Hayward , Simon Marchi References: <20210507084402.GA14817@delia> <977346CD-5DE9-49E0-A4C2-061792548857@arm.com> <187f89df-ff39-8522-1410-4c089c796698@polymtl.ca> <547E4B36-8597-46E9-9B5B-23B188BF896D@arm.com> From: Tom de Vries Message-ID: Date: Sat, 22 May 2021 10:32:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <547E4B36-8597-46E9-9B5B-23B188BF896D@arm.com> Content-Type: multipart/mixed; boundary="------------A22EF283DE46A83078963459" Content-Language: en-US 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: , Cc: ", palmer"@dabbelt.com, Andreas Arnez , nd , "gdb-patches\\@sourceware.org" Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" This is a multi-part message in MIME format. --------------A22EF283DE46A83078963459 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 5/21/21 12:50 PM, Alan Hayward wrote: >> I think it makes sense in any case to probe the main thread, we know >> it's always ok. >> > > ...But agreed this is the sensible option. Even if io_uring is ok, something else > could get added down the line which isn’t ok. Using pid should keep it safe. > Changes since last post: - added riscv64 (adding maintainers in cc) - added suggested change for arm - added comment at read_description in target.h Any further comments? Thanks, - Tom --------------A22EF283DE46A83078963459 Content-Type: text/x-patch; charset=UTF-8; name="0001-gdb-tdep-Use-pid-to-choose-process-64-32-bitness.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline; filename*0="0001-gdb-tdep-Use-pid-to-choose-process-64-32-bitness.patch" [gdb/tdep] Use pid to choose process 64/32-bitness In a linux kernel mailing list discussion, it was mentioned that "gdb has= this odd thing where it takes the 64-bit vs 32-bit data for the whole pro= cess from one thread, and picks the worst possible thread to do it (ie explici= tly not even the main thread, ...)" [1]. The picking of the thread is done here in x86_linux_nat_target::read_description: =2E.. /* GNU/Linux LWP ID's are process ID's. */ tid =3D inferior_ptid.lwp (); if (tid =3D=3D 0) tid =3D inferior_ptid.pid (); /* Not a threaded program. */ =2E.. To understand what this code does, let's investigate a scenario in which inferior_ptid.lwp () !=3D inferior_ptid.pid (). Say we start exec jit-attach-pie, identified with pid x. The main thread= starts another thread that sleeps, and then the main thread waits for the= sleeping thread. So we have two threads, identified with LWP IDs x and x= +1: =2E.. PID LWP CMD x x ./jit-attach-pie x x+1 ./jit-attach-pie =2E.. [ The thread with LWP x is known as the thread group leader. ] When attaching to this exec using the pid, gdb does a stop_all_threads wh= ich iterates over all the threads, first LWP x, and then LWP x+1. So the state we arrive with at x86_linux_nat_target::read_description is:= =2E.. (gdb) p inferior_ptid $1 =3D {m_pid =3D x, m_lwp =3D x+1, m_tid =3D 0} =2E.. and consequently we probe 64/32-bitness from thread LWP x+1. [ Note that this is different from when gdb doesn't attach but instead launches the exec itself, in which case there's just one thread to begin = with, and consequently the probed thread is LWP x. ] According to aforementioned remark, a better choice would have been the m= ain thread, that is, LWP x. This patch implement that choice, by simply doing: =2E.. tid =3D inferior_ptid.pid (); =2E.. The fact that gdb makes a per-process permanent choice for 64/32-bitness = is a problem in itself: each thread can be in either 64 or 32 bit mode, and ch= ange forth and back. That is a problem that this patch doesn't fix. Now finally: why does this matter in the context of the linux kernel discussion? The discussion was related to a patch that exposed io_uring threads to user-space. This made it possible that one of those threads w= ould be picked out to select 64/32-bitness. Given that such threads are atypi= cal user-space threads in the sense that they don't return to user-space and = don't have a userspace register state, reading their registers returns garbage,=20 and so it could f.i. occur that in a 64-bit process with all normal user-spac= e threads in 64-bit mode, the probing would return 32-bit. It may be that this is worked-around on the kernel side by providing user= space register state in those threads such that current gdb is happy. Neverthe= less, it seems prudent to fix this on the gdb size as well. Tested on x86_64-linux. [1] https://lore.kernel.org/io-uring/CAHk-=3Dwh0KoEZXPYMGkfkeVEerSCEF1AiC= ZSvz9TRrx=3DKj74D+Q@mail.gmail.com/ gdb/ChangeLog: 2021-05-07 Tom de Vries PR tdep/27822 * target.h (struct target_ops): Mention target_thread_architecture in read_description comment. * x86-linux-nat.c (x86_linux_nat_target::read_description): Use pid to determine if process is 64-bit or 32-bit. * aarch64-linux-nat.c (aarch64_linux_nat_target::read_description): Same. * ppc-linux-nat.c (ppc_linux_nat_target::read_description): Same. * riscv-linux-nat.c (riscv_linux_nat_target::read_description): S= ame. * s390-linux-nat.c (s390_linux_nat_target::read_description): Same. * arm-linux-nat.c (arm_linux_nat_target::read_description): Same. Likewise, use pid to determine if kernel supports reading VFP registers. --- gdb/aarch64-linux-nat.c | 2 +- gdb/arm-linux-nat.c | 4 ++-- gdb/ppc-linux-nat.c | 4 +--- gdb/riscv-linux-nat.c | 2 +- gdb/s390-linux-nat.c | 2 +- gdb/target.h | 5 ++++- gdb/x86-linux-nat.c | 5 +---- 7 files changed, 11 insertions(+), 13 deletions(-) diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index ae8db2988c2..61224022f6a 100644 --- a/gdb/aarch64-linux-nat.c +++ b/gdb/aarch64-linux-nat.c @@ -723,7 +723,7 @@ aarch64_linux_nat_target::read_description () gdb_byte regbuf[ARM_VFP3_REGS_SIZE]; struct iovec iovec; =20 - tid =3D inferior_ptid.lwp (); + tid =3D inferior_ptid.pid (); =20 iovec.iov_base =3D regbuf; iovec.iov_len =3D ARM_VFP3_REGS_SIZE; diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c index 662dade0a12..880ac0da044 100644 --- a/gdb/arm-linux-nat.c +++ b/gdb/arm-linux-nat.c @@ -537,7 +537,7 @@ arm_linux_nat_target::read_description () { elf_gregset_t gpregs; struct iovec iov; - int tid =3D inferior_ptid.lwp (); + int tid =3D inferior_ptid.pid (); =20 iov.iov_base =3D &gpregs; iov.iov_len =3D sizeof (gpregs); @@ -556,7 +556,7 @@ arm_linux_nat_target::read_description () { /* Make sure that the kernel supports reading VFP registers. Supp= ort was added in 2.6.30. */ - int pid =3D inferior_ptid.lwp (); + int pid =3D inferior_ptid.pid (); errno =3D 0; char *buf =3D (char *) alloca (ARM_VFP3_REGS_SIZE); if (ptrace (PTRACE_GETVFPREGS, pid, 0, buf) < 0 && errno =3D=3D EI= O) diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c index 171f5b386fa..06a30efeaef 100644 --- a/gdb/ppc-linux-nat.c +++ b/gdb/ppc-linux-nat.c @@ -1946,9 +1946,7 @@ ppc_linux_nat_target::auxv_parse (gdb_byte **readpt= r, const struct target_desc * ppc_linux_nat_target::read_description () { - int tid =3D inferior_ptid.lwp (); - if (tid =3D=3D 0) - tid =3D inferior_ptid.pid (); + int tid =3D inferior_ptid.pid (); =20 if (have_ptrace_getsetevrregs) { diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c index 04bf46b3bb1..c0f5a27a37e 100644 --- a/gdb/riscv-linux-nat.c +++ b/gdb/riscv-linux-nat.c @@ -202,7 +202,7 @@ const struct target_desc * riscv_linux_nat_target::read_description () { const struct riscv_gdbarch_features features - =3D riscv_linux_read_features (inferior_ptid.lwp ()); + =3D riscv_linux_read_features (inferior_ptid.pid ()); return riscv_lookup_target_description (features); } =20 diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c index 41b50ce4800..8f6eb61505b 100644 --- a/gdb/s390-linux-nat.c +++ b/gdb/s390-linux-nat.c @@ -988,7 +988,7 @@ s390_linux_nat_target::auxv_parse (gdb_byte **readptr= , const struct target_desc * s390_linux_nat_target::read_description () { - int tid =3D s390_inferior_tid (); + int tid =3D inferior_ptid.pid (); =20 have_regset_last_break =3D check_regset (tid, NT_S390_LAST_BREAK, 8); diff --git a/gdb/target.h b/gdb/target.h index d867a58e2a8..51139042691 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -841,7 +841,10 @@ struct target_ops /* Describe the architecture-specific features of this target. If OPS doesn't have a description, this should delegate to the "beneath" target. Returns the description found, or NULL if no - description was available. */ + description was available. This should return something that + describes a whole process, and if there are things that are + thread-specific, target_thread_architecture should be used for + that. */ virtual const struct target_desc *read_description () TARGET_DEFAULT_RETURN (NULL); =20 diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c index 85c7f0ddc94..adea1ad0092 100644 --- a/gdb/x86-linux-nat.c +++ b/gdb/x86-linux-nat.c @@ -113,10 +113,7 @@ x86_linux_nat_target::read_description () static uint64_t xcr0; uint64_t xcr0_features_bits; =20 - /* GNU/Linux LWP ID's are process ID's. */ - tid =3D inferior_ptid.lwp (); - if (tid =3D=3D 0) - tid =3D inferior_ptid.pid (); /* Not a threaded program. */ + tid =3D inferior_ptid.pid (); =20 #ifdef __x86_64__ { --------------A22EF283DE46A83078963459--