From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id E52883858CDB for ; Thu, 2 Mar 2023 18:28:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E52883858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.192] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 79B811E110; Thu, 2 Mar 2023 13:28:38 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1677781718; bh=avo511AN0mggPtqJEzO5rnQ5K8ti9EqdZxaowImJmIE=; h=Date:Subject:To:References:From:In-Reply-To:From; b=CtTjb3MjKbpqheK1H5BSEKyGnsWzVOLeJ2MCI6YryjfCPSfFH4inQyEWs2r9efU2P YYo9RJb8+6C8+ISD53Uc5SHKRpywaDhJHN5FhRnKru/dKkZB7heKbg3gN0suMIA3YL YIt6jx4GrbBr1ZN5kEj8JAh1zJsU/AJQI9svrINQ= Message-ID: <400e2248-0366-5b76-2e15-5e3a991c4d8c@simark.ca> Date: Thu, 2 Mar 2023 13:28:37 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH 2/2] gdb: add gdbarch::displaced_step_max_buffer_length Content-Language: fr To: Andrew Burgess , gdb-patches@sourceware.org References: <8b214d8ebfe903530c99e5c61c97021fd6a5ade6.1677602918.git.aburgess@redhat.com> From: Simon Marchi In-Reply-To: <8b214d8ebfe903530c99e5c61c97021fd6a5ade6.1677602918.git.aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,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 List-Id: On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote: > The gdbarch::max_insn_length field is used mostly to support displaced > stepping; it controls the size of the buffers allocated for the > displaced-step instruction, and is also used when first copying the > instruction, and later, when fixing up the instruction, in order to > read in and parse the instruction being stepped. > > However, it has started to be used in other places in GDB, for > example, it's used in the Python disassembler API, and it is used on > amd64 as part of branch-tracing instruction classification. > > The problem is that the value assigned to max_insn_length is not > always the maximum instruction length, but sometimes is a multiple of > that length, as required to support displaced stepping, see rs600, > ARM, and AArch64 for examples of this. > > It seems to me that we are overloading the meaning of the > max_insn_length field, and I think that could potentially lead to > confusion. > > I propose that we add a new gdbarch field, > gdbarch::displaced_step_max_buffer_length, this new field will do > exactly what it says on the tin; represent the required displaced step > buffer size. The max_insn_length field can then do exactly what it > claims to do; represent the maximum length of a single instruction. > > As some architectures (e.g. i386, and amd64) only require their > displaced step buffers to be a single instruction in size, I propose > that the default for displaced_step_max_buffer_length will be the > value of max_insn_length. Architectures than need more buffer space > can then override this default as needed. > > I've updated all architectures to setup the new field if appropriate, > and I've audited all calls to gdbarch_max_insn_length and switched to > gdbarch_displaced_step_max_buffer_length where appropriate. Naming nit: the displaced step buffer length isn't variable, so I don't think it makes sense to have "max" in the name. I think it should be named "displaced_step_buffer_length", since it's _the_ displaced step buffer length. > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py > index 1d420a513f9..a00398bb03d 100644 > --- a/gdb/gdbarch_components.py > +++ b/gdb/gdbarch_components.py > @@ -1752,7 +1752,10 @@ Advance PC to next instruction in order to skip a permanent breakpoint. > > Value( > comment=""" > -The maximum length of an instruction on this architecture in bytes. > +The maximum length of an instruction on this architecture in octets. > +This must be set for architectures that support displaced-stepping. > +Setting this for other architectures improves error detection within > +the Python disassembler API. I'm not sure I understand why you would mention that this needs to be set for architectures supporting displaced-stepping, since the point of your change is to use displaced_step_max_buffer_length for those use cases. These were only minor comments, the patch LGTM in general: Approved-By: Simon Marchi Simon