From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id F320B3858D1E for ; Tue, 7 May 2024 11:40:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F320B3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org F320B3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715082035; cv=none; b=Rgz++HBG2b6X300Z2GX/WTa6nFA4Ex1e4hy6j68P0rpk+H3kZkWTotH33RsfljcRQetTrp+CzTKE4w2tHlaIifG/VcHLXmfKkSSu2m3pP/FOsu6S4gMN3y73VkkTbevlewVreqLh/QGLXkGV4uTjtHpNeF+Bva9Sxpa8/UcXp2w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715082035; c=relaxed/simple; bh=d1bVHNBq7wM1dDdKG2Phxp1gPa/TEVLQSWKkOMwYz6w=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=NT/oeatHv5F0mCPNjaXWKmTUSwPayvFDwv7LFu0iFp06LnPafdFuX1cEKjtaS/2X/pdujs9UTTj+3tgXgzp7ynHYWcxMtAJHD8F/Hh67Vm3bCX44N72gA3Tw9BsUsRaxSZCAHron9jjfGHrT0pUa7m02LaAAk7kGQl9FfiKQd8c= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715082030; h=from:from:reply-to:subject:subject: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=exbKrGl44ALP5s7BXCY+Pb3cKwpTtkp5W6IkmhS7Zz8=; b=hCBEt/lvl+5QapIbyVxsxY5OYAsl6cWbeGbN5caODtAjyBlL1zF9I95eXNoSVcoZgHhp7L o4sUw2ehj1g7+8xLG53j7BdcOU/o2fMr9XEAbmhIQufcsqUfagohhnF2Q5etGQr26+sTLG de+hK0HOm/ypMpcFd0TfsyX4Z0e38Ig= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-108-Tyx5cC6YMJ6OnVLIYBEzjg-1; Tue, 07 May 2024 07:40:28 -0400 X-MC-Unique: Tyx5cC6YMJ6OnVLIYBEzjg-1 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-2e41bd70238so3344941fa.1 for ; Tue, 07 May 2024 04:40:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715082027; x=1715686827; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=exbKrGl44ALP5s7BXCY+Pb3cKwpTtkp5W6IkmhS7Zz8=; b=ttWIkwyhEb7DY7VXgwj8WwYD8iPJDceEns/rCoRIBIBf4v5R0mSIab84NHA0UXKQSL vxHWGTBeqIKw5TfamouPbTXWjUwH5VFVUVl5dF+C33OwOrCiXDYhrQgtN9YZhOr9YLCZ 71LfejL9I2uFALl/ZNHM/bYlzyLTAQyMEdQBMsP+1qnGhHtZp9qnPl5AdAbLXQQVUG8H Ci3WdO6XV8JGpwSwT5xsQ6IUkbv5Dh/7Kxc4JejpmBtgX3uuVQ0Ha4Jus9RGadgbLDOW DNTCwM7faJPyL+haNNWCPC6kllYH/5667YsQnCgRIBFcgn2AbdFhSxqO//YsLYtfL4c/ fDfw== X-Forwarded-Encrypted: i=1; AJvYcCUOyoLQpiyWlBVaCprgZ2N7z5STUDD9RKhRU9U7nd2THFo+/99GJXTqIWh2Vujzz61tSRsb8vykODghONbAJ7ACdf76Bp6Jv6UFkA== X-Gm-Message-State: AOJu0Ywji6zb0EhkziLxJFQGjL/N4wPQ0HcVMxSGyXrpcMZ+vKqnWoz5 0xagNoFat6HYkNZWKflE1Ve7DVgQZE5I2a25AmPx8iIrZU+6JWYsBtEWyZEEohWDaqPGC0rowmZ v0Ss1vwu4WPP83zU9J4NZz84fU2SG9UpnogACWPngrANXfXa/AQBKT+nFp24= X-Received: by 2002:a2e:8794:0:b0:2df:8387:ab70 with SMTP id n20-20020a2e8794000000b002df8387ab70mr8100696lji.14.1715082026678; Tue, 07 May 2024 04:40:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEy6YPyvPD0MNYPWeSJ1whE+gAvNDEtrtrtpfZI6quP5HnI6DnL/ve5SrQkXZJhgH4Wc9qmmw== X-Received: by 2002:a2e:8794:0:b0:2df:8387:ab70 with SMTP id n20-20020a2e8794000000b002df8387ab70mr8100671lji.14.1715082025854; Tue, 07 May 2024 04:40:25 -0700 (PDT) Received: from localhost ([31.111.84.240]) by smtp.gmail.com with ESMTPSA id bh10-20020a05600c3d0a00b0041c130520fbsm19273290wmb.46.2024.05.07.04.40.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 May 2024 04:40:25 -0700 (PDT) From: Andrew Burgess To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" Cc: John Baldwin Subject: RE: [PATCHv5 07/11] gdb/gdbserver: share some code relating to target description creation In-Reply-To: References: <885b8258b5651b4a4d70cd4cdcdb0c004ecd9fef.1714143669.git.aburgess@redhat.com> Date: Tue, 07 May 2024 12:40:24 +0100 Message-ID: <87zft1u9xz.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Willgerodt, Felix" writes: >> -----Original Message----- >> From: Andrew Burgess >> Sent: Freitag, 26. April 2024 17:02 >> To: gdb-patches@sourceware.org >> Cc: Andrew Burgess ; Willgerodt, Felix >> ; John Baldwin >> Subject: [PATCHv5 07/11] gdb/gdbserver: share some code relating to target >> description creation >> >> This commit is part of a series to share more of the x86 target >> description creation code between GDB and gdbserver. >> >> Unlike previous commits which were mostly refactoring, this commit is >> the first that makes a real change, though that change should mostly >> be for gdbserver; I've largely adopted the "GDB" way of doing things >> for gdbserver, and this fixes a real gdbserver bug. >> >> On a x86-64 Linux target, running the test: >> >> gdb.server/connect-with-no-symbol-file.exp >> >> results in two core files being created. Both of these core files are >> from the inferior process, created after gdbserver has detached. >> >> In this test a gdbserver process is started and then, after gdbserver >> has started, but before GDB attaches, we either delete the inferior >> executable, or change its permissions so it can't be read. Only after >> doing this do we attempt to connect with GDB. >> >> As GDB connects to gdbserver, gdbserver attempts to figure out the >> target description so that it can send the description to GDB, this >> involves a call to x86_linux_read_description. >> >> In x86_linux_read_description one of the first things we do is try to >> figure out if the process is 32-bit or 64-bit. To do this we look up >> the executable via the thread-id, and then attempt to read the >> architecture size from the executable. This isn't going to work if >> the executable has been deleted, or is no longer readable. >> >> And so, as we can't read the executable, we default to an i386 target >> and use an i386 target description. >> >> A consequence of using an i386 target description is that addresses >> are assumed to be 32-bits. Here's an example session that shows the >> problems this causes. This is run on an x86-64 machine, and the test >> binary (xx.x) is a standard 64-bit x86-64 binary: >> >> shell_1$ gdbserver --once localhost :54321 /tmp/xx.x >> >> shell_2$ gdb -q >> (gdb) set sysroot >> (gdb) shell chmod 000 /tmp/xx.x >> (gdb) target remote :54321 >> Remote debugging using :54321 >> warning: /tmp/xx.x: Permission denied. >> 0xf7fd3110 in ?? () >> (gdb) show architecture >> The target architecture is set to "auto" (currently "i386"). >> (gdb) p/x $pc >> $1 = 0xf7fd3110 >> (gdb) info proc mappings >> process 2412639 >> Mapped address spaces: >> >> Start Addr End Addr Size Offset Perms objfile >> 0x400000 0x401000 0x1000 0x0 r--p /tmp/xx.x >> 0x401000 0x402000 0x1000 0x1000 r-xp /tmp/xx.x >> 0x402000 0x403000 0x1000 0x2000 r--p /tmp/xx.x >> 0x403000 0x405000 0x2000 0x2000 rw-p /tmp/xx.x >> 0xf7fcb000 0xf7fcf000 0x4000 0x0 r--p [vvar] >> 0xf7fcf000 0xf7fd1000 0x2000 0x0 r-xp [vdso] >> 0xf7fd1000 0xf7fd3000 0x2000 0x0 r--p /usr/lib64/ld-2.30.so >> 0xf7fd3000 0xf7ff3000 0x20000 0x2000 r-xp /usr/lib64/ld-2.30.so >> 0xf7ff3000 0xf7ffb000 0x8000 0x22000 r--p /usr/lib64/ld-2.30.so >> 0xf7ffc000 0xf7ffe000 0x2000 0x2a000 rw-p /usr/lib64/ld-2.30.so >> 0xf7ffe000 0xf7fff000 0x1000 0x0 rw-p >> 0xfffda000 0xfffff000 0x25000 0x0 rw-p [stack] >> 0xff600000 0xff601000 0x1000 0x0 r-xp [vsyscall] >> (gdb) info inferiors >> Num Description Connection Executable >> * 1 process 2412639 1 (remote :54321) >> (gdb) shell cat /proc/2412639/maps >> 00400000-00401000 r--p 00000000 fd:03 45907133 /tmp/xx.x >> 00401000-00402000 r-xp 00001000 fd:03 45907133 /tmp/xx.x >> 00402000-00403000 r--p 00002000 fd:03 45907133 /tmp/xx.x >> 00403000-00405000 rw-p 00002000 fd:03 45907133 /tmp/xx.x >> 7ffff7fcb000-7ffff7fcf000 r--p 00000000 00:00 0 [vvar] >> 7ffff7fcf000-7ffff7fd1000 r-xp 00000000 00:00 0 [vdso] >> 7ffff7fd1000-7ffff7fd3000 r--p 00000000 fd:00 143904 /usr/lib64/ld-2.30.so >> 7ffff7fd3000-7ffff7ff3000 r-xp 00002000 fd:00 143904 /usr/lib64/ld-2.30.so >> 7ffff7ff3000-7ffff7ffb000 r--p 00022000 fd:00 143904 /usr/lib64/ld-2.30.so >> 7ffff7ffc000-7ffff7ffe000 rw-p 0002a000 fd:00 143904 /usr/lib64/ld-2.30.so >> 7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0 >> 7ffffffda000-7ffffffff000 rw-p 00000000 00:00 0 [stack] >> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] >> (gdb) >> >> Notice the difference between the mappings reported via GDB and those >> reported directly from the kernel via /proc/PID/maps, the addresses of >> every mapping is clamped to 32-bits for GDB, while the kernel reports >> real 64-bit addresses. >> >> Notice also that the $pc value is a 32-bit value. It appears to be >> within one of the mappings reported by GDB, but is outside any of the >> mappings reported from the kernel. >> >> And this is where the problem arises. When gdbserver detaches from >> the inferior we pass the inferior the address from which it should >> resume. Due to the 32/64 bit confusion we tell the inferior to resume >> from the 32-bit $pc value, which is not within any valid mapping, and >> so, as soon as the inferior resumes, it segfaults. >> >> If we look at how GDB (not gdbserver) figures out its target >> description then we see an interesting difference. GDB doesn't try to >> read the executable. Instead GDB uses ptrace to query the thread's >> state, and uses this to figure out the if the thread is 32 or 64 bit. >> >> If we update gdbserver to do it the "GDB" way then the above problem >> is resolved, gdbserver now sees the process as 64-bit, and when we >> detach from the inferior we give it the correct 64-bit address, and >> the inferior no longer segfaults. >> >> Now, I could just update the gdbserver code, but better, I think, to >> share one copy of the code between GDB and gdbserver in gdb/nat/. >> That is what this commit does. >> >> The cores of x86_linux_read_description from gdbserver and >> x86_linux_nat_target::read_description from GDB are moved into a new >> file gdb/nat/x86-linux-tdesc.c and combined into a single function >> x86_linux_tdesc_for_tid which is called from each location. >> >> This new function does things the GDB way, the only changes are to >> allow for the sharing; we now have a callback function to call the >> first time that the xcr0 state is read, this allows for GDB and >> gdbserver to perform their own initialisation as needed, and >> additionally, the new function takes a pointer for where to cache the >> xcr0 value, this isn't needed for this commit, but will be useful in a >> later commit where gdbserver will want to read this cached xcr0 >> value. >> >> Another thing to note about this commit is how the functions >> i386_linux_read_description and amd64_linux_read_description are >> handled. For now I've left these function as implemented separately >> in GDB and gdbserver. I've moved the declarations of these functions >> into gdb/arch/{i386,amd64}-linux-tdesc.h, but the implementations are >> left where they are. >> >> A later commit in this series will make these functions shared too, >> but doing this is not trivial, so I've left that for a separate >> commit. Merging the declarations as I've done here ensures that >> everyone implements the function to the same API, and once these >> functions are shared (in a later commit) we'll want a shared >> declaration anyway. >> --- >> gdb/Makefile.in | 3 + >> gdb/amd64-linux-tdep.c | 1 + >> gdb/amd64-linux-tdep.h | 6 -- >> gdb/arch/amd64-linux-tdesc.h | 30 +++++++ >> gdb/arch/i386-linux-tdesc.h | 29 +++++++ >> gdb/configure.nat | 4 +- >> gdb/i386-linux-tdep.c | 1 + >> gdb/i386-linux-tdep.h | 3 - >> gdb/nat/x86-linux-tdesc.c | 124 ++++++++++++++++++++++++++++ >> gdb/nat/x86-linux-tdesc.h | 60 ++++++++++++++ >> gdb/x86-linux-nat.c | 91 ++++----------------- >> gdbserver/configure.srv | 2 + >> gdbserver/linux-amd64-ipa.cc | 1 + >> gdbserver/linux-i386-ipa.cc | 1 + >> gdbserver/linux-x86-low.cc | 151 +++++++++++------------------------ >> gdbserver/linux-x86-tdesc.cc | 2 + >> gdbserver/linux-x86-tdesc.h | 7 -- >> 17 files changed, 317 insertions(+), 199 deletions(-) >> create mode 100644 gdb/arch/amd64-linux-tdesc.h >> create mode 100644 gdb/arch/i386-linux-tdesc.h >> create mode 100644 gdb/nat/x86-linux-tdesc.c >> create mode 100644 gdb/nat/x86-linux-tdesc.h >> >> diff --git a/gdb/Makefile.in b/gdb/Makefile.in >> index 353e45b3cec..a24b2232daa 100644 >> --- a/gdb/Makefile.in >> +++ b/gdb/Makefile.in >> @@ -1549,8 +1549,10 @@ HFILES_NO_SRCDIR = \ >> arch/aarch64-insn.h \ >> arch/aarch64-mte-linux.h \ >> arch/aarch64-scalable-linux.h \ >> + arch/amd64-linux-tdesc.h \ >> arch/arc.h \ >> arch/arm.h \ >> + arch/i386-linux-tdesc.h \ >> arch/i386.h \ >> arch/loongarch.h \ >> arch/ppc-linux-common.h \ >> @@ -1606,6 +1608,7 @@ HFILES_NO_SRCDIR = \ >> nat/x86-gcc-cpuid.h \ >> nat/x86-linux.h \ >> nat/x86-linux-dregs.h \ >> + nat/x86-linux-tdesc.h \ >> python/py-event.h \ >> python/py-events.h \ >> python/py-stopevent.h \ >> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c >> index 9d560ac4fbf..bcb9868e79e 100644 >> --- a/gdb/amd64-linux-tdep.c >> +++ b/gdb/amd64-linux-tdep.c >> @@ -41,6 +41,7 @@ >> #include "arch/amd64.h" >> #include "target-descriptions.h" >> #include "expop.h" >> +#include "arch/amd64-linux-tdesc.h" >> >> /* The syscall's XML filename for i386. */ >> #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml" >> diff --git a/gdb/amd64-linux-tdep.h b/gdb/amd64-linux-tdep.h >> index 2003dcda78f..0ec49e7fe03 100644 >> --- a/gdb/amd64-linux-tdep.h >> +++ b/gdb/amd64-linux-tdep.h >> @@ -43,12 +43,6 @@ extern struct target_desc *tdesc_x32_linux; >> extern struct target_desc *tdesc_x32_avx_linux; >> extern struct target_desc *tdesc_x32_avx_avx512_linux; >> >> -/* Return the right amd64-linux target descriptions according to >> - XCR0_FEATURES_BIT and IS_X32. */ >> - >> -const target_desc *amd64_linux_read_description (uint64_t xcr0_features_bit, >> - bool is_x32); >> - >> /* Enum that defines the syscall identifiers for amd64 linux. >> Used for process record/replay, these will be translated into >> a gdb-canonical set of syscall ids in linux-record.c. */ >> diff --git a/gdb/arch/amd64-linux-tdesc.h b/gdb/arch/amd64-linux-tdesc.h >> new file mode 100644 >> index 00000000000..db425b60df6 >> --- /dev/null >> +++ b/gdb/arch/amd64-linux-tdesc.h >> @@ -0,0 +1,30 @@ >> +/* Target description related code for GNU/Linux x86-64. >> + >> + Copyright (C) 2024 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#ifndef ARCH_AMD64_LINUX_TDESC_H >> +#define ARCH_AMD64_LINUX_TDESC_H >> + >> +struct target_desc; >> + >> +/* Return the AMD64 target descriptions corresponding to XCR0 and IS_X32. */ >> + >> +extern const target_desc *amd64_linux_read_description (uint64_t xcr0, >> + bool is_x32); >> + >> +#endif /* ARCH_AMD64_LINUX_TDESC_H */ >> diff --git a/gdb/arch/i386-linux-tdesc.h b/gdb/arch/i386-linux-tdesc.h >> new file mode 100644 >> index 00000000000..0b736337a75 >> --- /dev/null >> +++ b/gdb/arch/i386-linux-tdesc.h >> @@ -0,0 +1,29 @@ >> +/* Target description related code for GNU/Linux i386. >> + >> + Copyright (C) 2024 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#ifndef ARCH_I386_LINUX_TDESC_H >> +#define ARCH_I386_LINUX_TDESC_H >> + >> +struct target_desc; >> + >> +/* Return the i386 target description corresponding to XCR0. */ >> + >> +extern const struct target_desc *i386_linux_read_description (uint64_t xcr0); >> + >> +#endif /* ARCH_I386_LINUX_TDESC_H */ >> diff --git a/gdb/configure.nat b/gdb/configure.nat >> index 8b98511cef7..4bcc0696027 100644 >> --- a/gdb/configure.nat >> +++ b/gdb/configure.nat >> @@ -256,7 +256,7 @@ case ${gdb_host} in >> NATDEPFILES="${NATDEPFILES} x86-nat.o nat/x86-dregs.o \ >> nat/x86-xstate.o \ >> i386-linux-nat.o x86-linux-nat.o nat/linux-btrace.o \ >> - nat/x86-linux.o nat/x86-linux-dregs.o" >> + nat/x86-linux.o nat/x86-linux-dregs.o nat/x86-linux-tdesc.o" >> ;; >> ia64) >> # Host: Intel IA-64 running GNU/Linux >> @@ -322,7 +322,7 @@ case ${gdb_host} in >> NATDEPFILES="${NATDEPFILES} x86-nat.o nat/x86-dregs.o \ >> nat/x86-xstate.o amd64-nat.o amd64-linux-nat.o x86-linux-nat.o >> \ >> nat/linux-btrace.o \ >> - nat/x86-linux.o nat/x86-linux-dregs.o \ >> + nat/x86-linux.o nat/x86-linux-dregs.o nat/x86-linux-tdesc.o \ >> nat/amd64-linux-siginfo.o" >> ;; >> sparc) >> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c >> index 44730f204db..78ebc99d3df 100644 >> --- a/gdb/i386-linux-tdep.c >> +++ b/gdb/i386-linux-tdep.c >> @@ -39,6 +39,7 @@ >> >> #include "i387-tdep.h" >> #include "gdbsupport/x86-xstate.h" >> +#include "arch/i386-linux-tdesc.h" >> >> /* The syscall's XML filename for i386. */ >> #define XML_SYSCALL_FILENAME_I386 "syscalls/i386-linux.xml" >> diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h >> index 07593c6a8ec..e8691cd778e 100644 >> --- a/gdb/i386-linux-tdep.h >> +++ b/gdb/i386-linux-tdep.h >> @@ -55,9 +55,6 @@ extern void i386_linux_report_signal_info (struct gdbarch >> *gdbarch, >> struct ui_out *uiout, >> enum gdb_signal siggnal); >> >> -/* Return the target description according to XCR0. */ >> -extern const struct target_desc *i386_linux_read_description (uint64_t xcr0); >> - >> extern int i386_linux_gregset_reg_offset[]; >> >> /* Return x86 siginfo type. */ >> diff --git a/gdb/nat/x86-linux-tdesc.c b/gdb/nat/x86-linux-tdesc.c >> new file mode 100644 >> index 00000000000..c2301e6a873 >> --- /dev/null >> +++ b/gdb/nat/x86-linux-tdesc.c >> @@ -0,0 +1,124 @@ >> +/* Target description related code for GNU/Linux x86 (i386 and x86-64). >> + >> + Copyright (C) 2024 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#include "nat/x86-linux-tdesc.h" >> +#ifdef __x86_64__ >> +#include "arch/amd64.h" >> +#include "arch/amd64-linux-tdesc.h" >> +#endif >> +#include "arch/i386.h" >> +#include "arch/i386-linux-tdesc.h" >> + >> +#include "nat/x86-linux.h" >> +#include "nat/gdb_ptrace.h" >> +#include "nat/x86-xstate.h" >> + >> +#ifndef __x86_64__ >> +#include >> +#endif >> + >> +#include >> +#include >> + >> +#ifndef IN_PROCESS_AGENT >> + >> +/* See nat/x86-linux-tdesc.h. */ >> + >> +const target_desc * >> +x86_linux_tdesc_for_tid (int tid, enum tribool *have_ptrace_getregset, >> + gdb::function_view xcr0_init_cb, >> + const char *error_msg, uint64_t *xcr0_storage) >> +{ >> +#ifdef __x86_64__ >> + >> + x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid); >> + bool is_64bit = arch_size.is_64bit (); >> + bool is_x32 = arch_size.is_x32 (); >> + >> + if (sizeof (void *) == 4 && is_64bit && !is_x32) >> + error ("%s", error_msg); >> + >> +#elif HAVE_PTRACE_GETFPXREGS >> + if (have_ptrace_getfpxregs == -1) >> + { >> + elf_fpxregset_t fpxregs; >> + >> + if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0) >> + { >> + have_ptrace_getfpxregs = 0; >> + *have_ptrace_getregset = TRIBOOL_FALSE; > > We tried PTRACE_GETFPXREGS, does that really indicate > PTRACE_GETREGSET is not supported? I realize you just moved this code, > but it feels weird to me. Then again this is Linux specific and it might actually > be the case. Thanks for all your feedback on this series. I'm working through the comments, but wanted to reply to this point separately: I also wondered about this. I initially left if because, as you say, I'm just moving the code, and didn't want to change too much in one go. Given your feedback though I did trace this code back to this commit: 3a13a53b432d732be258d677b6afca969ce0d65f from April 2010. What's interesting is this part of the ChangeLog: (i386_linux_read_description): Set have_ptrace_getfpxregs and have_ptrace_getregset to 0 if ptrace PTRACE_GETFPXREGS failed. The function name has changed since then, but notice that the author specifically mentions their intention to set BOTH flags when PTRACE_GETFPXREGS fails. Based on that I'm inclined to leave this alone unless it can be shown that it's wrong. Thanks, Andrew > > >> + return i386_linux_read_description (X86_XSTATE_X87_MASK); >> + } >> + } >> +#endif >> + >> + if (*have_ptrace_getregset == TRIBOOL_UNKNOWN) >> + { >> + uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))]; >> + struct iovec iov; >> + >> + iov.iov_base = xstateregs; >> + iov.iov_len = sizeof (xstateregs); >> + >> + /* Check if PTRACE_GETREGSET works. */ >> + if (ptrace (PTRACE_GETREGSET, tid, >> + (unsigned int) NT_X86_XSTATE, &iov) < 0) >> + { >> + *have_ptrace_getregset = TRIBOOL_FALSE; >> + *xcr0_storage = 0; >> + } >> + else >> + { >> + *have_ptrace_getregset = TRIBOOL_TRUE; >> + >> + /* Get XCR0 from XSAVE extended state. */ >> + *xcr0_storage = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET >> + / sizeof (uint64_t))]; >> + >> +#ifdef __x86_64__ >> + /* No MPX on x32. */ >> + if (is_64bit && is_x32) >> + *xcr0_storage &= ~X86_XSTATE_MPX; >> +#endif /* __x86_64__ */ >> + >> + xcr0_init_cb (*xcr0_storage); > > > Needing a callback made me scratch my head. What could be the reason > that GDB and gdbserver really want to do different things here? > > Digging into the code made me see that GDB saves just the xsave_layout. > Gdbserver in addition saves xcr0, though I guess that might not even > be needed if you have a tdesc already (or the xsave_layout?). > And they are saving things in different locations (global in gdbserver, > member in GDB), but that could be done without a callback I think. > > I realize that changing these things is a bigger tasks, that you might > not want to do at all or mix with this series. But can we avoid > a callback? E.g. by adding another return value, that only gdbserver > uses for now? With a TODO comment explaining this? > > > >> + } >> + } >> + >> + /* Check the native XCR0 only if PTRACE_GETREGSET is available. If >> + PTRACE_GETREGSET is not available then set xcr0_features_bits to >> + zero so that the "no-features" descriptions are returned by the >> + switches below. */ >> + uint64_t xcr0_features_bits; >> + if (*have_ptrace_getregset == TRIBOOL_TRUE) >> + xcr0_features_bits = *xcr0_storage & X86_XSTATE_ALL_MASK; >> + else >> + xcr0_features_bits = 0; >> + >> +#ifdef __x86_64__ >> + if (is_64bit) >> + { >> + return amd64_linux_read_description (xcr0_features_bits, is_x32); >> + } > > Nit: I think we could just drop the brackets here. > >> + else >> +#endif >> + return i386_linux_read_description (xcr0_features_bits); >> +} >> + >> +#endif /* !IN_PROCESS_AGENT */ >> diff --git a/gdb/nat/x86-linux-tdesc.h b/gdb/nat/x86-linux-tdesc.h >> new file mode 100644 >> index 00000000000..8c481609876 >> --- /dev/null >> +++ b/gdb/nat/x86-linux-tdesc.h >> @@ -0,0 +1,60 @@ >> +/* Target description related code for GNU/Linux x86 (i386 and x86-64). >> + >> + Copyright (C) 2024 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#ifndef NAT_X86_LINUX_TDESC_H >> +#define NAT_X86_LINUX_TDESC_H >> + >> +#include "gdbsupport/function-view.h" >> + >> +struct target_desc; >> + >> +/* Return the target description for Linux thread TID. >> + >> + When *HAVE_PTRACE_GETREGSET is TRIBOOL_UNKNOWN then the current >> value of >> + xcr0 is read using ptrace calls and stored into *XCR0_STORAGE. Then >> + XCR0_INIT_CB is called with the value of *XCR0_STORAGE and >> + *HAVE_PTRACE_GETREGSET is set to TRIBOOL_TRUE. >> + >> + If the attempt to read xcr0 using ptrace fails then *XCR0_STORAGE is set >> + to zero and *HAVE_PTRACE_GETREGSET is set to TRIBOOL_FALSE. >> + >> + The storage pointed to by XCR0_STORAGE must exist until the program >> + terminates, this storage is used to cache the xcr0 value. As such >> + XCR0_INIT_CB will only be called once if xcr0 is successfully read using >> + ptrace, or not at all if the ptrace call fails. >> + >> + This function returns a target description based on the extracted xcr0 >> + value along with other characteristics of the thread identified by TID. >> + >> + This function can return nullptr if we encounter a machine configuration >> + for which a target_desc cannot be created. Ideally this would not be >> + the case, we should be able to create a target description for every >> + possible machine configuration. See amd64_linux_read_description and >> + i386_linux_read_description for cases when nullptr might be >> + returned. >> + >> + ERROR_MSG is using in an error() call if we try to create a target >> + description for a 64-bit process but this is a 32-bit build of GDB. */ >> + >> +extern const target_desc * >> +x86_linux_tdesc_for_tid (int tid, enum tribool *have_ptrace_getregset, >> + gdb::function_view xcr0_init_cb, >> + const char *error_msg, uint64_t *xcr0_storage); >> + >> +#endif /* NAT_X86_LINUX_TDESC_H */ >> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c >> index 12965c7a21b..9fbccb28d21 100644 >> --- a/gdb/x86-linux-nat.c >> +++ b/gdb/x86-linux-nat.c >> @@ -38,6 +38,7 @@ >> #include "nat/x86-linux.h" >> #include "nat/x86-linux-dregs.h" >> #include "nat/linux-ptrace.h" >> +#include "nat/x86-linux-tdesc.h" >> >> /* linux_nat_target::low_new_fork implementation. */ >> >> @@ -92,90 +93,26 @@ x86_linux_nat_target::post_startup_inferior (ptid_t ptid) >> const struct target_desc * >> x86_linux_nat_target::read_description () >> { >> - int tid; >> - int is_64bit = 0; >> -#ifdef __x86_64__ >> - int is_x32; >> -#endif >> - static uint64_t xcr0; >> - uint64_t xcr0_features_bits; >> + static uint64_t xcr0_storage; >> >> if (inferior_ptid == null_ptid) >> return this->beneath ()->read_description (); >> >> - tid = inferior_ptid.pid (); >> - >> -#ifdef __x86_64__ >> - >> - x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid); >> - is_64bit = arch_size.is_64bit (); >> - is_x32 = arch_size.is_x32 (); >> - >> -#elif HAVE_PTRACE_GETFPXREGS >> - if (have_ptrace_getfpxregs == -1) >> - { >> - elf_fpxregset_t fpxregs; >> - >> - if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0) >> - { >> - have_ptrace_getfpxregs = 0; >> - have_ptrace_getregset = TRIBOOL_FALSE; >> - return i386_linux_read_description (X86_XSTATE_X87_MASK); >> - } >> - } >> -#endif >> - >> - if (have_ptrace_getregset == TRIBOOL_UNKNOWN) >> - { >> - uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))]; >> - struct iovec iov; >> - >> - iov.iov_base = xstateregs; >> - iov.iov_len = sizeof (xstateregs); >> - >> - /* Check if PTRACE_GETREGSET works. */ >> - if (ptrace (PTRACE_GETREGSET, tid, >> - (unsigned int) NT_X86_XSTATE, &iov) < 0) >> - have_ptrace_getregset = TRIBOOL_FALSE; >> - else >> - { >> - have_ptrace_getregset = TRIBOOL_TRUE; >> - >> - /* Get XCR0 from XSAVE extended state. */ >> - xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET >> - / sizeof (uint64_t))]; >> - >> - m_xsave_layout = x86_fetch_xsave_layout (xcr0, x86_xsave_length ()); >> - } >> - } >> - >> - /* Check the native XCR0 only if PTRACE_GETREGSET is available. If >> - PTRACE_GETREGSET is not available then set xcr0_features_bits to >> - zero so that the "no-features" descriptions are returned by the >> - switches below. */ > > Should we s/switches/code? > >> - if (have_ptrace_getregset == TRIBOOL_TRUE) >> - xcr0_features_bits = xcr0 & X86_XSTATE_ALL_MASK; >> - else >> - xcr0_features_bits = 0; >> - >> - if (is_64bit) >> - { >> -#ifdef __x86_64__ >> - return amd64_linux_read_description (xcr0_features_bits, is_x32); >> -#endif >> - } >> - else >> - { >> - const struct target_desc * tdesc >> - = i386_linux_read_description (xcr0_features_bits); >> + int tid = inferior_ptid.pid (); >> >> - if (tdesc == NULL) >> - tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK); >> + const char *error_msg >> + = _("Can't debug 64-bit process with 32-bit GDB"); >> >> - return tdesc; >> - } >> + /* Callback that is triggered the first time x86_linux_tdesc_for_tid >> + reads the xcr0 register. Setup other bits of state. */ >> + auto cb = [&] (uint64_t xcr0) >> + { >> + this->m_xsave_layout >> + = x86_fetch_xsave_layout (xcr0, x86_xsave_length ()); >> + }; >> >> - gdb_assert_not_reached ("failed to return tdesc"); >> + return x86_linux_tdesc_for_tid (tid, &have_ptrace_getregset, cb, >> + error_msg, &xcr0_storage); >> } >> >> >> >> >> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv >> index 9e861a75088..7a2702d78bf 100644 >> --- a/gdbserver/configure.srv >> +++ b/gdbserver/configure.srv >> @@ -109,6 +109,7 @@ case "${gdbserver_host}" in >> srv_tgtobj="${srv_tgtobj} nat/linux-btrace.o" >> srv_tgtobj="${srv_tgtobj} nat/x86-linux.o" >> srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o" >> + srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o" >> srv_linux_usrregs=yes >> srv_linux_regsets=yes >> srv_linux_thread_db=yes >> @@ -371,6 +372,7 @@ case "${gdbserver_host}" in >> srv_tgtobj="${srv_tgtobj} nat/linux-btrace.o" >> srv_tgtobj="${srv_tgtobj} nat/x86-linux.o" >> srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o" >> + srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o" >> srv_tgtobj="${srv_tgtobj} nat/amd64-linux-siginfo.o" >> srv_linux_usrregs=yes # This is for i386 progs. >> srv_linux_regsets=yes >> diff --git a/gdbserver/linux-amd64-ipa.cc b/gdbserver/linux-amd64-ipa.cc >> index a6346750f49..df5e6aca081 100644 >> --- a/gdbserver/linux-amd64-ipa.cc >> +++ b/gdbserver/linux-amd64-ipa.cc >> @@ -22,6 +22,7 @@ >> #include "tracepoint.h" >> #include "linux-x86-tdesc.h" >> #include "gdbsupport/x86-xstate.h" >> +#include "arch/amd64-linux-tdesc.h" >> >> /* fast tracepoints collect registers. */ >> >> diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc >> index 8f14e0937d4..aa346fc9bc3 100644 >> --- a/gdbserver/linux-i386-ipa.cc >> +++ b/gdbserver/linux-i386-ipa.cc >> @@ -22,6 +22,7 @@ >> #include "tracepoint.h" >> #include "linux-x86-tdesc.h" >> #include "gdbsupport/x86-xstate.h" >> +#include "arch/i386-linux-tdesc.h" >> >> /* GDB register numbers. */ >> >> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc >> index 62612773a87..68d2f13537c 100644 >> --- a/gdbserver/linux-x86-low.cc >> +++ b/gdbserver/linux-x86-low.cc >> @@ -29,8 +29,11 @@ >> >> #ifdef __x86_64__ >> #include "nat/amd64-linux-siginfo.h" >> +#include "arch/amd64-linux-tdesc.h" >> #endif >> >> +#include "arch/i386-linux-tdesc.h" >> + >> #include "gdb_proc_service.h" >> /* Don't include elf/common.h if linux/elf.h got included by >> gdb_proc_service.h. */ >> @@ -46,6 +49,7 @@ >> #include "nat/x86-linux.h" >> #include "nat/x86-linux-dregs.h" >> #include "linux-x86-tdesc.h" >> +#include "nat/x86-linux-tdesc.h" >> >> #ifdef __x86_64__ >> static target_desc_up tdesc_amd64_linux_no_xml; >> @@ -831,32 +835,20 @@ x86_target::low_siginfo_fixup (siginfo_t *ptrace, >> gdb_byte *inf, int direction) >> >> >> >> static int use_xml; >> >> +/* Cached xcr0 value. This is initialised the first time >> + x86_linux_read_description is called. */ >> + >> +static uint64_t xcr0_storage; > > We already cache it in fp-i387.cc. It get's set by the callback > you added. I think we should avoid having it twice. > > >> /* Get Linux/x86 target description from running target. */ >> >> static const struct target_desc * >> x86_linux_read_description (void) > > Not strictly necessary, but we could remove the void while we are touching this. > This is a .cc file. > > Thanks, > Felix > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928