From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sGm+E2pu/GXKhBAAWB0awg (envelope-from ) for ; Thu, 21 Mar 2024 13:29:14 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; secure) header.d=freebsd.org header.i=@freebsd.org header.a=rsa-sha256 header.s=dkim header.b=X8qEM4Xk; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 4BA4D1E0C0; Thu, 21 Mar 2024 13:29:14 -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 CB8891E08C for ; Thu, 21 Mar 2024 13:29:11 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2DF06385841A for ; Thu, 21 Mar 2024 17:29:11 +0000 (GMT) Received: from mx2.freebsd.org (mx2.freebsd.org [96.47.72.81]) by sourceware.org (Postfix) with ESMTPS id 630083858D28 for ; Thu, 21 Mar 2024 17:28:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 630083858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 630083858D28 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=96.47.72.81 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1711042128; cv=pass; b=VL0VWk44AYRuo5gMzeIbw6gutwegSpFRnJz9+Kn/L4tHYG6iPihJy7Rdl3H1rBT0sEthmovu1Gp2WiI+eFb9psX0GMGNC7Nw/gU33Ht2k4gtfzztmIpXqY4AkYQtyLxiOEgCvhqWTzQwIf2zNLgKGC40AMZ0Vs5mIq+xxsu3ZiU= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1711042128; c=relaxed/simple; bh=aqz5SQDqE/uF0etSDS9Dzv3oUkGfIPRIYoTrkkmbQsc=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=cjs+yuyQAWiv9iBccaIWDogO73E/UhM2PUOG3drobfRBrdK7kPeGscyzCw4F+oTHGh5CYQ3nvvZccyOh1o/SqFhgtnqX2arw8ClWLzhusSpVlyVDk1mKGtsDQWSe/T5oascaU1xL9IhgtFLueCFm8nsE44f6PblMW8KGbJ1vRhQ= ARC-Authentication-Results: i=2; server2.sourceware.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4V0srG0DNmz3w8D; Thu, 21 Mar 2024 17:28:46 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4V0srF4q0Yz432F; Thu, 21 Mar 2024 17:28:45 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1711042125; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=c+qXP8GVPWhlQAJRX7MXqsl2kp2N8e9PpY+R5tnIwXQ=; b=X8qEM4XkE6grTwD8aH5TKdZywRAs2jI83FvttY35TxZFjoDu7+LFPC0/VREZb+sFGQU9us 6utlIrOSpqpMiDfTQdEExUVCg3QkHQM6Z6SqtWpqOJyrRxiNCzHbP5yY8OgV8qygEWiExx vbZ7xO9xw8G770KCqnL54APC2QEpcpDTjiThSOMg0/7F6zWSVxYcW8/ORP1+8319MdcebT KmP1ZpW486GQrwJAEQJrMU+zoaH5dUNTr9+mLjnnKGyFo+gLJFiYFymYnMFmd5hUx4hBYX ezwOJENbaX6rWGHk41fdRtSDoyN5yIQ2vN59gkQrYLomfqGTnQSwiyDdFVZQJg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1711042125; a=rsa-sha256; cv=none; b=yJaLo8BxgI2ORmx82JtvBz7H+HXs7JlnLa+c+oOvbaqRG2i6SOtnogeDX2wPgjAVDzLnIq mypvtcKiiQrEPqTQa9qIx0LArfYKnIn2Q1ME8UmTc9PV9014Lwo+D9F25ow5G8GOEqm0rQ b5loljk9qZSpb6J7YHWnB4EytzIJ30EHCsIQXTuLCZSpB9uGjc7+8+oMo59EjXp+I3CTJi dNxWT1GV+NBqhAEWTJErZjCQ5bayLtfVOwrULO/jdQ7NACuJuff6UdHy8He/no9lJbqlBf 0G0MfWSz48R0oSMoNVVfY5+9ua58agxBA+UmUlXTxKSl3l4V7QNBJXDlEaRPkA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1711042125; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=c+qXP8GVPWhlQAJRX7MXqsl2kp2N8e9PpY+R5tnIwXQ=; b=O8FT0s38Y2K3Ah2+HLDfeJwcQSV85sydsdqgGwxzrlnC9WLipxI12m27UMMlt3nBO7HwD4 r/7wCNlv7vMyhBR2Mi3RwnxdpOxTy9dg6CFoxNULoNS6zJhC77csM3QXU1Dj2YR7lEmIs8 ELD5h9A0wl4vmOqbeUEdV8RFkIKjoTazlzQ4XE5HMeNHmEuiRTnJYCBlJS1cBrMBZYYw+c xUqrs8wvFO5SeFAFkcoSYr28voWKN0BmoRYIwaZk03+ilKQ2Qs5vsVuzSwcM4r241UAAF5 FKBB4ZfwN8ieNgdnuCxyf09rWENQVj8dB4mp+2nnH/ztA8mU+X9s3C09tsy0mw== Received: from [128.232.109.28] (user-109-28.vpn.cl.cam.ac.uk [128.232.109.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4V0srD6PHXz14DC; Thu, 21 Mar 2024 17:28:44 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Thu, 21 Mar 2024 10:28:41 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCHv2 6/7] gdbserver: update target description creation for x86/linux Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <87plvqt6jl.fsf@redhat.com> From: John Baldwin In-Reply-To: <87plvqt6jl.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, 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 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 On 3/19/24 11:34 AM, Andrew Burgess wrote: > 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. I have zero knowledge about the IPA myself. OTOH, if no one else steps up with a strong opinion here, I would suggest that you go ahead and fix the IPA as you described so it is more self-contained and then we can use a saner approach for dealing with tdesc caching for x86. -- John Baldwin