From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id G8weMrza+WXp8A0AWB0awg (envelope-from ) for ; Tue, 19 Mar 2024 14:34:36 -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=NgZ0j4+z; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id B8BDE1E0BB; Tue, 19 Mar 2024 14:34:36 -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 9936B1E08C for ; Tue, 19 Mar 2024 14:34:34 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2E7FA3858C31 for ; Tue, 19 Mar 2024 18:34:34 +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 E21853858CD1 for ; Tue, 19 Mar 2024 18:34:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E21853858CD1 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 E21853858CD1 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=1710873253; cv=none; b=E5+S5AvaDSdg66iNcrFIubFOeGVHrLLuSCeXqZ8NfVfNIRIAMYI3dtsQFqZhBdanEarnLJPnEPJykEvsZIICQnH0jyhB6X8/+CvhvJOb7ZJz8W9zPrXfvmJJACyAibUgfUWTjhSO8Za4cERBTbOnUgJojdcBilMoU57CKjbmxFk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710873253; c=relaxed/simple; bh=bS2E8iYHCZB80wUEWIHK5Yq6aE1s2nz4qpBp+g+NTp4=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=W9eged06dgllRtKLsPo7atevxxFdnTv8S3FpDDpZ3SXaXejUX2+Qvggb+M0zMpnD1LZz2RZn90WzH0J0JRhgC/1Awby1qHob8aI4Z1NuIalyR4TH5WyNLAcfDHHHwP7WKo1tM7FCNBrhQDw4Y6dmhUfFWmveVSpUKFZD2JAGp24= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1710873251; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=WFZiKp1aYqA8UlXHfEo2K5Qcu6G2hSXDXAqI9geAQH4=; b=NgZ0j4+zrR9wZCc9AMHNHJMYDAcmCvDOdSiZGqrpvMIjPgFK0mHuy/CK9kMyUIeKRFJTjp WCZoVDxk17pMyAsTqPGlooytF+TS/jt0WJ/LqZB2RByfGg2bg/TVkMpg6gFdVtAxrbwdwT IgwPydIkp1NZbojwp+1nsBE/a/PfehY= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-110-JigA-f-pMXGL9-eq4L5KKg-1; Tue, 19 Mar 2024 14:34:10 -0400 X-MC-Unique: JigA-f-pMXGL9-eq4L5KKg-1 Received: by mail-ed1-f71.google.com with SMTP id 4fb4d7f45d1cf-5689f41cf4dso3141678a12.3 for ; Tue, 19 Mar 2024 11:34:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710873248; x=1711478048; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WFZiKp1aYqA8UlXHfEo2K5Qcu6G2hSXDXAqI9geAQH4=; b=tbIl/AjGszJkZ7VtkiqNXXNXrQUFMPepFXONtcPqz0zMtDn5Ha/EfsBaZO0AWexw0w newNB7E/jFrPrclLyUOWrzB+cX4AiKegEOOnbV/kfJdnzueoWWilEjGBOoR6a91pUXCz fge/D7GvTxh54htKOIzQG84qqGnUBBCSZwknG3uQG+ORW7nGWqHAUUAgC0BC0LXikVPJ F+hyqfal360ugEC7+VVjRrQqTtolS4mRc87G4D6QLg2l9fZCBKeuFkzCr55jZ6GZyDdb zmZ/nDwtci4qckL1bj0tOk1WXQPGpUwWao5ifuRHtJsiQ3uBpecb4UVEg2PTkTDuoi0+ lY+Q== X-Forwarded-Encrypted: i=1; AJvYcCW3JNH7sMppUN8EB7Zmcx4dZ4oN6Ij6Y7WjmaviNdqVbmjdv/V0c7sjd5GL2uLgpEmexSGtsHMzK1XNRC8jEIh5CgSUY2TrzY9rvw== X-Gm-Message-State: AOJu0YzPefNtnmuG7BydkkeRfaOShM5KafFhBbDR9aI1ELOgrrpXGqvt 2PxOlaQHg+bRtOi9j5m81tkymauUjSbOYaYD/BuIyzewl5fvTd5VDfZW7telHaecC5o3SsfEld2 sGN3bu7s1G0UggzHtCgtPNmClDpwIpx66DYfEuuZZceSboH3LtXJsH/BiettR/CAd930= X-Received: by 2002:a05:6402:c4e:b0:568:9ba8:8f14 with SMTP id cs14-20020a0564020c4e00b005689ba88f14mr2867921edb.24.1710873248024; Tue, 19 Mar 2024 11:34:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGO9dcc9Vr4AdKdXXFN/aElwwzRz7IzoJEIEKToWw5oqRGt07oDIsycNfbg0fDiZC6rFB4k+g== X-Received: by 2002:a05:6402:c4e:b0:568:9ba8:8f14 with SMTP id cs14-20020a0564020c4e00b005689ba88f14mr2867905edb.24.1710873247492; Tue, 19 Mar 2024 11:34:07 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id ef6-20020a05640228c600b00568c2ea2cefsm3772836edb.51.2024.03.19.11.34.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Mar 2024 11:34:07 -0700 (PDT) From: Andrew Burgess To: John Baldwin , gdb-patches@sourceware.org Subject: Re: [PATCHv2 6/7] gdbserver: update target description creation for x86/linux In-Reply-To: References: Date: Tue, 19 Mar 2024 18:34:06 +0000 Message-ID: <87plvqt6jl.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=-6.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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 John Baldwin writes: > On 3/5/24 9:00 AM, Andrew Burgess wrote: >> This commit is part of a series which aims to share more of the target >> description creation between GDB and gdbserver for x86/Linux. >> >> After some refactoring, the previous commit actually started to share >> some code, we added the shared x86_linux_tdesc_for_tid function into >> nat/x86-linux-tdesc.c. However, this function still relies on >> amd64_linux_read_description and i386_linux_read_description which are >> implemented separately for both gdbserver and GDB. Given that at >> their core, all these functions to is: >> >> 1. take an xcr0 value as input, >> 2. mask out some feature bits, >> 3. look for a cached pre-generated target description and return it >> if found, >> 4. if no cached target description is found then call either >> amd64_create_target_description or >> i386_create_target_description to create a new target >> description, which is then added to the cache. Return the newly >> created target description. >> >> The inner functions amd64_create_target_description and >> i386_create_target_description are already shared between GDB and >> gdbserver (in the gdb/arch/ directory), so the only thing that >> the *_read_description functions really do is add the caching layer, >> and it feels like this really could be shared. >> >> However, we have a small problem. >> >> On the GDB side we create target descriptions using a different set of >> cpu features than on the gdbserver side! This means that for the >> exact same target, we might get a different target description when >> using native GDB vs using gdbserver. This surely feels like a >> mistake, I would expect to get the same target description on each. >> >> The table below shows the number of possible different target >> descriptions that we can create on the GDB side vs on the gdbserver >> side for each target type: >> >> | GDB | gdbserver >> ------|-----|---------- >> i386 | 64 | 7 >> amd64 | 32 | 7 >> x32 | 16 | 7 >> >> So in theory, all I want to do is move the GDB version >> of *_read_description into the nat/ directory and have gdbserver use >> that, then both GDB and gdbserver would be able to create any of the >> possible target descriptions. >> >> Unfortunately it's a little more complex than that due to the in >> process agent (IPA). >> >> When the IPA is in use, gdbserver sends a target description index to >> the IPA, and the IPA uses this to find the correct target description >> to use. >> >> ** START OF AN ASIDE ** >> >> Back in the day I suspect this approach made perfect sense. However >> since this commit: >> >> commit a8806230241d201f808d856eaae4d44088117b0c >> Date: Thu Dec 7 17:07:01 2017 +0000 >> >> Initialize target description early in IPA >> >> I think passing the index is now more trouble than its worth. >> >> We used to pass the index, and then use that index to lookup which >> target description to instantiate and use. However, the above commit >> fixed an issue where we can't call malloc() within (certain parts of) >> the IPA (apparently), so instead we now pre-compute _every_ possible >> target description within the IPA. The index is now only used to >> lookup which of the (many) pre-computed target descriptions to use. >> >> It would (I think) have been easier all around if the IPA just >> self-inspected, figured out its own xcr0 value, and used that to >> create the one target description that is required. So long as the >> xcr0 to target description code is shared (at compile time) with >> gdbserver, then we can be sure that the IPA will derive the same >> target description as gdbserver, and we would avoid all this index >> passing business, which has made this commit so very, very painful. >> >> ** END OF AN ASIDE ** >> >> Currently then for x86/linux, gdbserver sends a number between 0 and 7 >> to the IPA, and the IPA uses this to create a target description. >> >> However, I am proposing that gdbserver should now create one of (up >> to) 64 different target descriptions for i386, so this 0 to 7 index >> isn't going to be good enough any more (amd64 and x32 have slightly >> fewer possible target descriptions, but still more than 8, so the >> problem is the same). >> >> For a while I wondered if I was going to have to try and find some >> backward compatible solution for this mess. But after seeing how >> lightly the IPA is actually documented, I wonder if it is not the case >> that there is a tight coupling between a version of gdbserver and a >> version of the IPA? At least I'm hoping so. >> >> In this commit I have thrown out the old IPA target description index >> numbering scheme, and switched to a completely new numbering scheme. >> Instead of the index that is passed being arbitrary, the index is >> instead calculated from the set of cpu features that are present on >> the target. Within the IPA we can then reverse this logic to recreate >> the xcr0 value based on the index, and from the xcr0 value we can >> create the correct target description. >> >> With the gdbserver to IPA numbering scheme issue resolved I have then >> update the gdbserver versions of amd64_linux_read_description and >> i386_linux_read_description so that they create target descriptions >> using the same set of cpu features as GDB itself. >> >> After this gdbserver should now always come up with the same target >> description as GDB does on any x86/Linux target. >> >> This commit does not introduce any new code sharing between GDB and >> gdbserver as previous commits in this series does. Instead this >> commit is all about bringing GDB and gdbserver into alignment >> functionally so that the next commit can merge the GDB and gdbserver >> versions of these functions. > > It does seem like the IPA case should just compute the tdesc it needs > if I understand you correctly. Yes, totally. And it's not like it would even add much code to the IPA, I believe it already pulls in all of the target description creation code, the only thing that is "saved" is the code that probes the hardware to figure out which description we should use. This whole area seems like it has not aged well :-/ > > BTW, on other arches like aarch64 and I believe risc-v, a flat array > is not used for the tdesc cache. Now the cache is a std::unordered_map<> > and there is a 'struct foo_features' that has an operator== and > std::hash<> specialization. It really would be nice if x86 was the same, > but that would require abandoning a notion of a portable "index" shared > between gdbserver and the IPA. Indeed. I did the worked on the feature/hash/lookup for RISC-V so I had this in mind when looking at this code. Originally I had planned to switch to a map style cache, but when I found the IPA/index passing code I lost my nerve and tried to find a solution that was inline with what we had currently. I don't really know about the IPA enough to make claims about what is/isn't a reasonable change in this area. Thanks, Andrew