From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id oP+cHNSaYGbHcicAWB0awg (envelope-from ) for ; Wed, 05 Jun 2024 13:05:24 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=QIiK5bH2; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 639241E0C0; Wed, 5 Jun 2024 13:05:24 -0400 (EDT) Received: from server2.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 ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 3F8CF1E092 for ; Wed, 5 Jun 2024 13:05:22 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CA3853919BFD for ; Wed, 5 Jun 2024 17:05:21 +0000 (GMT) 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 5ECAE3858C56 for ; Wed, 5 Jun 2024 17:04:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5ECAE3858C56 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 5ECAE3858C56 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=1717607099; cv=none; b=d0VAsAV67VusJ5rLljNnMp2rGmQ18U61rTBCsYOCn71BSwNcKSYgRoIprBFxv3Xg3IMDTFtcrdqGg62iE974NqzN2jUYJdyzV4cRh6rcnSjQa4kHzJclWUAuqBix8Taq0+/UvFqmi7npb2XfdfU9UWb/dtrNx/hA7nVYTIejTCY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717607099; c=relaxed/simple; bh=toocma47PqSgJSDP1fNlOxdR4XdKmOJAujHkLoctmjo=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=pozo+bx8ZI58SWIwO6KTLXbmbwe0S0H4DkeLrhSpZq0UrqCQ83XDVWvtsydXLCRV2BdQ0fxI46A9CIKMnF/Kq0Gi52daW/7SV2ggrqNsQ259pr2FHmVbJnv/k+ny+/BqnjhmD3i4oBs2k9p0jXe9yYG5BihUwoEJCk16KopDQe0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1717607097; 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=cI1UmkQP1nibphGj+MXI7kqefzHtRH21Mz3DA3FLmBM=; b=QIiK5bH232GiwFQokTs03oo4EucaclP6EiepaBV07lFCICEgYShuGGGw8tjq3rkT3xvzLy VGokCtgRdwgQvxsXn45/NmBvrT/eGB8Xu6igER/JD/9i7TwhtTRJhKtHReyG9RzMn4TZh+ wVZvC2wq3H2GpXvDrS2YowqubzksyfM= Received: from mail-lj1-f198.google.com (mail-lj1-f198.google.com [209.85.208.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-554-DU29O1ZTPHC-nF7_ZoNB-Q-1; Wed, 05 Jun 2024 13:04:55 -0400 X-MC-Unique: DU29O1ZTPHC-nF7_ZoNB-Q-1 Received: by mail-lj1-f198.google.com with SMTP id 38308e7fff4ca-2eaaae3e600so899021fa.0 for ; Wed, 05 Jun 2024 10:04:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717607094; x=1718211894; 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=cI1UmkQP1nibphGj+MXI7kqefzHtRH21Mz3DA3FLmBM=; b=WXr+5JLFUO0D949rJjuQIWIAdqtZTu0vJeq77XXrLlpvVeBxw0KJRDRVlOnyPO4y/g ktZODOUruH+9ABmyQuppm3h2UY/DqK8pt5EL4kbrqjHLauxgEcxn4Oo/9WqmWycRUiOq k2i4B3x+7x+9mG0XfLXDxPB0koHKcStnrqJMCVgRrqJe1zTctDDFRxPFsrP/OJBTB/bH y2sTTLLG/9Ecz05qsANNkdmLbRlxBJD2/TncasdUAYwnbytWuhscQuT0rGydzXsQAsS/ Id73Dtid888Ge4cxRegp+6U+MTPEAm6WjVKYETLppPn25Mbd/b7z2x8iohyHFn6sD56R Qufw== X-Forwarded-Encrypted: i=1; AJvYcCXIhC5MPS+5UAltII1ictpxIWP/Mh/g4o6myyChntI2wmejvyla3m2v+Xv2qL5ubrTem/xkUTZtcdo/rX966HESYxEd5nuJDB/ZbQ== X-Gm-Message-State: AOJu0YzSg6dPnAFG1Y+ZM+342El+MygxYZbHyB/tVgGS4f66JFKgebvc s9Pw1YvZEhWpE+f3M1qVV/f9qWIdVeZLpwFLHSa8FVkeJ1VXX1TZHQJ3mY3GiDzW2g1BdDMujlQ VnZDDbmLMKVKiOhDd6C39Yhe0R6w+Z6/yCbB2nHX4FAM4SDhc11/S/1jI+NE= X-Received: by 2002:a2e:b0cf:0:b0:2ea:c65d:d9d0 with SMTP id 38308e7fff4ca-2eac7a715a4mr22800931fa.33.1717607094046; Wed, 05 Jun 2024 10:04:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEbK7433pDYY0MWZTLG1DYuceYHBtUZ6hkeKPSiWNEk1p2yL01itiG54eXVh62h/odTRFgAhQ== X-Received: by 2002:a2e:b0cf:0:b0:2ea:c65d:d9d0 with SMTP id 38308e7fff4ca-2eac7a715a4mr22800691fa.33.1717607093413; Wed, 05 Jun 2024 10:04:53 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42158116e41sm27999655e9.21.2024.06.05.10.04.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Jun 2024 10:04:52 -0700 (PDT) From: Andrew Burgess To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" Cc: "jhb@FreeBSD.org" Subject: RE: [PATCHv7 7/9] gdb/gdbserver: share some code relating to target description creation In-Reply-To: References: <3f2e66bab7bb82afcc7cd5297786807c6626f2d6.1715421687.git.aburgess@redhat.com> Date: Wed, 05 Jun 2024 18:04:52 +0100 Message-ID: <87o78fb9sb.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=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org "Willgerodt, Felix" writes: > Hi Andrew, > > I mainly have some nits and one actual remark (the x32 check). > Thanks again for doing this and sorry for taking a bit to review this. No problem. I've not been pushing this series as I didn't want to land this before the release branched off, I think this should be tested in master for a while before making it into a release. I have some follow up, mostly on the x32 checks... >> + >> +const target_desc * >> +x86_linux_tdesc_for_tid (int tid, const char *error_msg, >> + uint64_t *xcr0_storage, >> + x86_xsave_layout *xsave_layout_storage) >> +{ >> +#ifdef __x86_64__ >> + > > It looks weird to me that there is an empty line here but not for the others. Fixed. >> +#include "gdbsupport/function-view.h" >> + >> +struct target_desc; >> +struct x86_xsave_layout; >> + >> +/* Return the target description for Linux thread TID. >> + >> + The storage pointed to by XCR0_STORAGE AND XSAVE_LAYOUT_STORAGE >> must > > s/AND/and Fixed. >> diff --git a/gdbserver/i387-fp.h b/gdbserver/i387-fp.h >> index 450466efb75..4ee21da8461 100644 >> --- a/gdbserver/i387-fp.h >> +++ b/gdbserver/i387-fp.h >> @@ -19,6 +19,8 @@ >> #ifndef GDBSERVER_I387_FP_H >> #define GDBSERVER_I387_FP_H >> >> +struct x86_xsave_layout; > > Do we really save much from not including the header and doing > a forward declaration? But I guess GDB is already super inconsistent > on that front. I don't know. There's plenty of places where we forward declare like this. I thought we only added the header if we needed the contents of the struct. I've left this as is for now, but if it really bothers you I can change it, I'm not that attached to this style. >> @@ -836,29 +840,12 @@ static int use_xml; >> /* Get Linux/x86 target description from running target. */ >> >> static const struct target_desc * >> -x86_linux_read_description (void) >> +x86_linux_read_description () >> { >> - unsigned int machine; >> - int is_elf64; >> - int xcr0_features; >> - int tid; >> - static uint64_t xcr0; >> - static int xsave_len; >> - struct regset_info *regset; >> - >> - tid = lwpid_of (current_thread); >> + int tid = lwpid_of (current_thread); >> >> - is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine); >> - >> - if (sizeof (void *) == 4) >> - { >> - if (is_elf64 > 0) >> - error (_("Can't debug 64-bit process with 32-bit GDBserver")); >> -#ifndef __x86_64__ >> - else if (machine == EM_X86_64) >> - error (_("Can't debug x86-64 process with 32-bit GDBserver")); >> -#endif >> - } >> + const char *error_msg >> + = _("Can't debug 64-bit process with 32-bit GDBserver"); >> >> /* If we are not allowed to send an XML target description then we need >> to use the hard-wired target descriptions. This corresponds to GDB's >> @@ -868,103 +855,53 @@ x86_linux_read_description (void) >> generate some alternative target descriptions. */ >> if (!use_xml) >> { >> + 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); > > I am stumbling a bit on these x32 checks. So, we do this check twice in code > now anyway? If we did this unconditionally here (and once in GDB code) > we could get rid of this check and the error_msg argument in > x86_linux_tdesc_for_tid(). And still have this code just twice. > > Factoring it out into a separate function would also be something that comes > to mind, though it might be less easy to do, I didn't really check. > > Or maybe I am missing something. No you've not missed anything, this is not ideal right now. So my argument for duplicating this code is that the duplication is inside the use_xml block. One thing I'm planning to propose as a follow up to this series is to removed the use_xml flag and all the associated code, including this duplicate error check. The use_xml flag is only set to 0 if a GDB connects to gdbserver and doesn't offer the 'xmlRegisters' feature in its qSupported packet, this means that GDB was compiled without XML support. My argument for removing this code (spoiler alert!) is going to be: 1. Having two fixed tdesc to x86, with all its possible variants, doesn't make a lot of sense, the fixed tdesc are unlikely to be correct. 2. GDB has had xml support for 10+ years now, so I don't think there's really any reason why users should be using a version of GDB without xml register support to connect to gdbserver. One argument that I've heard is: "What about compiling GDB on small targets where the XML libraries are not available?" My thinking is that you'd only compile on such a target in order to run GDB natively on that target. You wouldn't then connect from that target to some remote. If you want remote debug then compile on a full sized machine where the xml libraries are available and connect to the remote from there. 3. I'd then remove the use_xml flag and all associated code, and add an error check: if a GDB connects to an x86 gdbserver, and GDB doesn't offer the 'xmlRegisters' feature, gdbserver would give an error and drop the connection. With that in mind I've tried throughout this series to keep the use_xml code separate from the reset of the code. Then on a purely engineering level, it's not entirely clear what would be a good way too share the code. Moving the error check out of the function isn't great, we then need to document that checking for this error is a prerequisite, plus we'd still need to call x86_linux_ptrace_get_arch_size within x86_linux_read_description as we need the result for other things. We could move the error check to a helper function, but either the helper needs to call x86_linux_ptrace_get_arch_size itself, which means we repeat that work, or the caller of the helper has to call x86_linux_ptrace_get_arch_size, in which case the helper function is really just a wrapper around: if (sizeof (void *) == 4 && is_64bit && !is_x32) error (_("Can't debug 64-bit process with 32-bit GDBserver")); which maybe isn't terrible, but isn't a huge win. I haven't changed anything here for now, but if you really would like to see some sharing happen then I can add a smaller helper function, it's not much effort. >> + >> + /* Use PTRACE_GETREGSET if it is available. */ >> + for (regset_info *regset = x86_regsets; >> + regset->fill_function != nullptr; >> + regset++) >> + if (regset->get_request == PTRACE_GETREGSET) >> + regset->size = xsave_len; >> + else if (regset->type != GENERAL_REGS) >> + regset->size = 0; > > I know you copied this, but doesn't this somehow go against our coding > standards? Not having braces for loops that have more then one line? > Though we don't have this particular case written down it seems, only for > if statements. Fixed. I'll post another iteration of this series once I've finished updating and testing this new version. Thanks, Andrew