From patchwork Mon Mar 6 15:31:57 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 66034 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 028E438515F1 for ; Mon, 6 Mar 2023 15:33:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 028E438515F1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1678116780; bh=WPiRKQOYcxJ1o6Dfiu+HvAjM8+A+kd+k+X71grt8GpE=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=VOww5KnIGzyX/yju8tq0/3RSZCCZrZ0/LeAf7RYV4mfQOgbZlvN94g6W63N9XCs7m BWlAipp9n/9DNsdR1jeQskaThJB4xmV/JCV44cLtbhWUFjzOeXVTC1rci39oDp6hlq yarYOWDTtXlxGG1Ypv24phQn2a9Nsr2oF2Vku4oQ= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 6F21D385B526 for ; Mon, 6 Mar 2023 15:32:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F21D385B526 Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-157-emdZRvDWN5KAflMhmehzqA-1; Mon, 06 Mar 2023 10:32:09 -0500 X-MC-Unique: emdZRvDWN5KAflMhmehzqA-1 Received: by mail-ed1-f70.google.com with SMTP id d35-20020a056402402300b004e37aed9832so5003092eda.18 for ; Mon, 06 Mar 2023 07:32:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678116728; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WPiRKQOYcxJ1o6Dfiu+HvAjM8+A+kd+k+X71grt8GpE=; b=tqo2N/yeJsle2aOxkcJyJ6C3nf6STa5J/IR83Oko5UGLwz5mcUg45HrjVQBEQHQH6u L0PL6BxcWINsoXLVdsfk0kXeOpwkzJRQ7j7zmJromNMOTFfWbotloBltN/ZzNKDD+56G KeB2iWMNGEtLXys8Emp4KeN3fjoHOme75Fxzy/WQZojCz3FaZkmnEuepbuGB8+jrMkE7 FmHLzKEK6dks2PXg+00g1J+JFFaYUxpJ6yu+HoKzAI8p61dH2GMNkBQFK2ogekOOLNLP RK5Fg3Ov2BsxV6VRZEcKGyuPBEuNN9P46oT250bxOvazzObkXh1qG/Zv+BKlfcH5hc2n /75A== X-Gm-Message-State: AO0yUKXgvc9IkIrGPEaMd1C4pJBl+LjgdIZh4kgf55VNkTHOUPv8i2Li 4/FUgRBYbJBd5vVPccMsIrlPQEdmeN913Htwy5WkTa4DsA3LlK/H7p6Z2vfUcnNWb+cvC+lvQGN IQze+lTGRnkP6x4uRBt1kdedx/5hD8BXzskvCWacblz9bo5RlJTsR89FPWLFMllpF3HonXROppz 0KUFrFJw== X-Received: by 2002:a17:907:60d1:b0:907:68b9:e05c with SMTP id hv17-20020a17090760d100b0090768b9e05cmr14346605ejc.58.1678116727640; Mon, 06 Mar 2023 07:32:07 -0800 (PST) X-Google-Smtp-Source: AK7set+ErpuXXO6jlZR4tG+KRMFWR+nhz9fFkX9DPz8IGxrEnbiy2F/7c7czMR8oUBaIf05cZ93CBQ== X-Received: by 2002:a17:907:60d1:b0:907:68b9:e05c with SMTP id hv17-20020a17090760d100b0090768b9e05cmr14346572ejc.58.1678116727287; Mon, 06 Mar 2023 07:32:07 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id 23-20020a170906005700b008e3e2b6a9adsm4734112ejg.94.2023.03.06.07.32.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Mar 2023 07:32:06 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess , Simon Marchi Subject: [PATCHv2 5/5] gdb: add gdbarch::displaced_step_buffer_length Date: Mon, 6 Mar 2023 15:31:57 +0000 Message-Id: <4d9f698341b49f6f0c58537a46caf30b953d17ca.1678116328.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.7 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_H2, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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_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_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_buffer_length where appropriate. There should be no user visible changes after this commit. Approved-By: Simon Marchi --- gdb/aarch64-linux-tdep.c | 4 +++- gdb/arm-tdep.c | 4 +++- gdb/displaced-stepping.c | 6 +++--- gdb/gdbarch-gen.h | 12 ++++++++++-- gdb/gdbarch.c | 26 ++++++++++++++++++++++++++ gdb/gdbarch_components.py | 18 ++++++++++++++++-- gdb/linux-tdep.c | 2 +- gdb/rs6000-tdep.c | 6 ++++-- 8 files changed, 66 insertions(+), 12 deletions(-) diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c index 0000b498f89..3eaf4e1131f 100644 --- a/gdb/aarch64-linux-tdep.c +++ b/gdb/aarch64-linux-tdep.c @@ -2240,7 +2240,9 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number); /* Displaced stepping. */ - set_gdbarch_max_insn_length (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS); + set_gdbarch_max_insn_length (gdbarch, 4); + set_gdbarch_displaced_step_max_buffer_length + (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS); set_gdbarch_displaced_step_copy_insn (gdbarch, aarch64_displaced_step_copy_insn); set_gdbarch_displaced_step_fixup (gdbarch, aarch64_displaced_step_fixup); diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index b64c21ce68f..d1941992c77 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -10662,7 +10662,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) /* Note: for displaced stepping, this includes the breakpoint, and one word of additional scratch space. This setting isn't used for anything beside displaced stepping at present. */ - set_gdbarch_max_insn_length (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS); + set_gdbarch_displaced_step_max_buffer_length + (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS); + set_gdbarch_max_insn_length (gdbarch, 4); /* This should be low enough for everything. */ tdep->lowest_pc = 0x20; diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c index 06b32a80f6a..72b6366390f 100644 --- a/gdb/displaced-stepping.c +++ b/gdb/displaced-stepping.c @@ -55,7 +55,7 @@ displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc) regcache *regcache = get_thread_regcache (thread); const address_space *aspace = regcache->aspace (); gdbarch *arch = regcache->arch (); - ULONGEST len = gdbarch_max_insn_length (arch); + ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch); /* Search for an unused buffer. */ displaced_step_buffer *buffer = nullptr; @@ -243,7 +243,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, below. */ thread->inf->displaced_step_state.unavailable = false; - ULONGEST len = gdbarch_max_insn_length (arch); + ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch); /* Restore memory of the buffer. */ write_memory_ptid (thread->ptid, buffer->addr, @@ -302,7 +302,7 @@ displaced_step_buffers::restore_in_ptid (ptid_t ptid) regcache *regcache = get_thread_regcache (buffer.current_thread); gdbarch *arch = regcache->arch (); - ULONGEST len = gdbarch_max_insn_length (arch); + ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch); write_memory_ptid (ptid, buffer.addr, buffer.saved_copy.data (), len); diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index ddb97f60315..76d12a15317 100644 --- a/gdb/gdbarch-gen.h +++ b/gdb/gdbarch-gen.h @@ -1039,8 +1039,8 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i see the comments in infrun.c. The TO area is only guaranteed to have space for - gdbarch_max_insn_length (arch) bytes, so this function must not - write more bytes than that to that area. + gdbarch_displaced_step_buffer_length (arch) octets, so this + function must not write more octets than that to this area. If you do not provide this function, GDB assumes that the architecture does not support displaced stepping. @@ -1122,6 +1122,14 @@ typedef void (gdbarch_displaced_step_restore_all_in_ptid_ftype) (inferior *paren extern void gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, inferior *parent_inf, ptid_t child_ptid); extern void set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid); +/* The maximum length in octets required for a displaced-step instruction + buffer. By default this will be the same as gdbarch::max_insn_length, + but should be overridden for architectures that might expand a + displaced-step instruction to multiple replacement instructions. */ + +extern ULONGEST gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch); +extern void set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch, ULONGEST displaced_step_buffer_length); + /* Relocate an instruction to execute at a different address. OLDLOC is the address in the inferior memory where the instruction to relocate is currently at. On input, TO points to the destination diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index efd111eeabc..8cb307d39c9 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -192,6 +192,7 @@ struct gdbarch gdbarch_displaced_step_finish_ftype *displaced_step_finish = NULL; gdbarch_displaced_step_copy_insn_closure_by_addr_ftype *displaced_step_copy_insn_closure_by_addr = nullptr; gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid = nullptr; + ULONGEST displaced_step_buffer_length = 0; gdbarch_relocate_instruction_ftype *relocate_instruction = NULL; gdbarch_overlay_update_ftype *overlay_update = nullptr; gdbarch_core_read_description_ftype *core_read_description = nullptr; @@ -463,6 +464,10 @@ verify_gdbarch (struct gdbarch *gdbarch) log.puts ("\n\tdisplaced_step_finish"); /* Skip verify of displaced_step_copy_insn_closure_by_addr, has predicate. */ /* Skip verify of displaced_step_restore_all_in_ptid, invalid_p == 0 */ + if (gdbarch->displaced_step_buffer_length == 0) + gdbarch->displaced_step_buffer_length = gdbarch->max_insn_length; + if (gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length) + log.puts ("\n\tdisplaced_step_buffer_length"); /* Skip verify of relocate_instruction, has predicate. */ /* Skip verify of overlay_update, has predicate. */ /* Skip verify of core_read_description, has predicate. */ @@ -1121,6 +1126,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file) gdb_printf (file, "gdbarch_dump: displaced_step_restore_all_in_ptid = <%s>\n", host_address_to_string (gdbarch->displaced_step_restore_all_in_ptid)); + gdb_printf (file, + "gdbarch_dump: displaced_step_buffer_length = %s\n", + plongest (gdbarch->displaced_step_buffer_length)); gdb_printf (file, "gdbarch_dump: gdbarch_relocate_instruction_p() = %d\n", gdbarch_relocate_instruction_p (gdbarch)); @@ -4169,6 +4177,24 @@ set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, gdbarch->displaced_step_restore_all_in_ptid = displaced_step_restore_all_in_ptid; } +ULONGEST +gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch) +{ + gdb_assert (gdbarch != NULL); + /* Check variable is valid. */ + gdb_assert (!(gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length)); + if (gdbarch_debug >= 2) + gdb_printf (gdb_stdlog, "gdbarch_displaced_step_buffer_length called\n"); + return gdbarch->displaced_step_buffer_length; +} + +void +set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch, + ULONGEST displaced_step_buffer_length) +{ + gdbarch->displaced_step_buffer_length = displaced_step_buffer_length; +} + bool gdbarch_relocate_instruction_p (struct gdbarch *gdbarch) { diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py index 1eef2fb584e..58fba1aea71 100644 --- a/gdb/gdbarch_components.py +++ b/gdb/gdbarch_components.py @@ -1739,8 +1739,8 @@ For a general explanation of displaced stepping and how GDB uses it, see the comments in infrun.c. The TO area is only guaranteed to have space for -gdbarch_max_insn_length (arch) bytes, so this function must not -write more bytes than that to that area. +gdbarch_displaced_step_buffer_length (arch) octets, so this +function must not write more octets than that to this area. If you do not provide this function, GDB assumes that the architecture does not support displaced stepping. @@ -1848,6 +1848,20 @@ contents of all displaced step buffers in the child's address space. invalid=False, ) +Value( + comment=""" +The maximum length in octets required for a displaced-step instruction +buffer. By default this will be the same as gdbarch::max_insn_length, +but should be overridden for architectures that might expand a +displaced-step instruction to multiple replacement instructions. +""", + type="ULONGEST", + name="displaced_step_buffer_length", + predefault="0", + postdefault="gdbarch->max_insn_length", + invalid="gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length", +) + Method( comment=""" Relocate an instruction to execute at a different address. OLDLOC diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index e6ce13a1c67..ce48515d8ca 100644 --- a/gdb/linux-tdep.c +++ b/gdb/linux-tdep.c @@ -2603,7 +2603,7 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, at DISP_STEP_BUF_ADDR. They are all of size BUF_LEN. */ CORE_ADDR disp_step_buf_addr = linux_displaced_step_location (thread->inf->gdbarch); - int buf_len = gdbarch_max_insn_length (arch); + int buf_len = gdbarch_displaced_step_max_buffer_length (arch); linux_gdbarch_data *gdbarch_data = get_linux_gdbarch_data (arch); gdb_assert (gdbarch_data->num_disp_step_buffers > 0); diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 104515de030..9fc4574bb41 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -889,7 +889,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs) { - size_t len = gdbarch_max_insn_length (gdbarch); + size_t len = gdbarch_displaced_step_max_buffer_length (gdbarch); + gdb_assert (len > PPC_INSN_SIZE); std::unique_ptr closure (new ppc_displaced_step_copy_insn_closure (len)); gdb_byte *buf = closure->buf.data (); @@ -8363,8 +8364,9 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish); set_gdbarch_displaced_step_restore_all_in_ptid (gdbarch, ppc_displaced_step_restore_all_in_ptid); + set_gdbarch_displaced_step_max_buffer_length (gdbarch, 2 * PPC_INSN_SIZE); - set_gdbarch_max_insn_length (gdbarch, 2 * PPC_INSN_SIZE); + set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE); /* Hook in ABI-specific overrides, if they have been registered. */ info.target_desc = tdesc;