From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id +ccwE1/VqGD9EwAAWB0awg (envelope-from ) for ; Sat, 22 May 2021 05:56:47 -0400 Received: by simark.ca (Postfix, from userid 112) id 42AFC1E54D; Sat, 22 May 2021 05:56:47 -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 C69371E54D for ; Sat, 22 May 2021 05:56:45 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 54735384802C; Sat, 22 May 2021 09:56:45 +0000 (GMT) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 37C163857C77 for ; Sat, 22 May 2021 09:56:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 37C163857C77 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=1621677401; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4oFsTVgtK6PRH7hPQiro5XDb4T5Qh7RpxwKZSuIueRw=; b=UWn0VAQMvA/yHLM6p5LT8zu0huFDn3POsv72UZhHbNj67Yb4wyMl0RDSxAt4Y9QBqkILr4 K9R0d3cTWGOTlmeUhCje6l74Tbgx16nW2ifp9Yv7DnSGAKmkCZXH+ZReBxJbbxuoN62V1V 3HQ1tOH4BHXHCWN6OVoUZCtmLkeEvTQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1621677401; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4oFsTVgtK6PRH7hPQiro5XDb4T5Qh7RpxwKZSuIueRw=; b=KpVIyEtwdeuXTcCS6E4VOwSIqxwf8qtIf1AGN7ik5pxD2O19VnH0aLZpdG2P1ucY5HCMVd yolxxUhTip5jUMDQ== Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 2A7A6AD2D; Sat, 22 May 2021 09:56:41 +0000 (UTC) Subject: Re: [PATCH][gdb/tdep] Use pid to choose x86_64 process 64/32-bitness From: Tom de Vries 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> Message-ID: Date: Sat, 22 May 2021 11:56:40 +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: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Andreas Arnez , nd , "gdb-patches\\@sourceware.org" Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" [ correcting mistyped email address ] On 5/22/21 10:32 AM, Tom de Vries wrote: > 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 > > > 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 process > from one thread, and picks the worst possible thread to do it (ie explicitly > not even the main thread, ...)" [1]. > > The picking of the thread is done here in > x86_linux_nat_target::read_description: > ... > /* GNU/Linux LWP ID's are process ID's. */ > tid = inferior_ptid.lwp (); > if (tid == 0) > tid = inferior_ptid.pid (); /* Not a threaded program. */ > ... > > To understand what this code does, let's investigate a scenario in which > inferior_ptid.lwp () != 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: > ... > PID LWP CMD > x x ./jit-attach-pie > x x+1 ./jit-attach-pie > ... > [ 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 which > 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: > ... > (gdb) p inferior_ptid > $1 = {m_pid = x, m_lwp = x+1, m_tid = 0} > ... > 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 main > thread, that is, LWP x. > > This patch implement that choice, by simply doing: > ... > tid = inferior_ptid.pid (); > ... > > 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 change > 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 would > be picked out to select 64/32-bitness. Given that such threads are atypical > 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, > and > so it could f.i. occur that in a 64-bit process with all normal user-space > 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 userspace > register state in those threads such that current gdb is happy. Nevertheless, > 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-=wh0KoEZXPYMGkfkeVEerSCEF1AiCZSvz9TRrx=Kj74D+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): Same. > * 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; > > - tid = inferior_ptid.lwp (); > + tid = inferior_ptid.pid (); > > iovec.iov_base = regbuf; > iovec.iov_len = 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 = inferior_ptid.lwp (); > + int tid = inferior_ptid.pid (); > > iov.iov_base = &gpregs; > iov.iov_len = sizeof (gpregs); > @@ -556,7 +556,7 @@ arm_linux_nat_target::read_description () > { > /* Make sure that the kernel supports reading VFP registers. Support was > added in 2.6.30. */ > - int pid = inferior_ptid.lwp (); > + int pid = inferior_ptid.pid (); > errno = 0; > char *buf = (char *) alloca (ARM_VFP3_REGS_SIZE); > if (ptrace (PTRACE_GETVFPREGS, pid, 0, buf) < 0 && errno == EIO) > 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 **readptr, > const struct target_desc * > ppc_linux_nat_target::read_description () > { > - int tid = inferior_ptid.lwp (); > - if (tid == 0) > - tid = inferior_ptid.pid (); > + int tid = inferior_ptid.pid (); > > 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 > - = riscv_linux_read_features (inferior_ptid.lwp ()); > + = riscv_linux_read_features (inferior_ptid.pid ()); > return riscv_lookup_target_description (features); > } > > 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 = s390_inferior_tid (); > + int tid = inferior_ptid.pid (); > > have_regset_last_break > = 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); > > 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; > > - /* GNU/Linux LWP ID's are process ID's. */ > - tid = inferior_ptid.lwp (); > - if (tid == 0) > - tid = inferior_ptid.pid (); /* Not a threaded program. */ > + tid = inferior_ptid.pid (); > > #ifdef __x86_64__ > { >