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 86270384C005 for ; Thu, 6 Aug 2020 14:55:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 86270384C005 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 2725E1E594; Thu, 6 Aug 2020 10:55:39 -0400 (EDT) Subject: Re: [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf To: Victor Collod , gdb-patches@sourceware.org References: <0cc93067-1313-6434-4330-61a21736376f@simark.ca> <20200624012857.31849-1-vcollod@nvidia.com> <20200624012857.31849-4-vcollod@nvidia.com> From: Simon Marchi Message-ID: <2585517f-5149-2325-1aa9-959e5068d1d2@simark.ca> Date: Thu, 6 Aug 2020 10:55:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200624012857.31849-4-vcollod@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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-List-Received-Date: Thu, 06 Aug 2020 14:55:40 -0000 On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote: > Both variables were used to store function code. I expressed concerns earlier that the code might be written in this way on purpose, to make sure we don't read too much, in case we are close to the end of a readable region. I don't know if this is something that can actually happen in pracptice, but still tt makes sense to me that we read a single byte, check what it is, and read more if needed. Any performance concerns should be taken care of by some cache at the lower level (each call to read_code won't necessarily cause a target read to happen). > 2020-06-23 Victor Collod > > * amd64-tdep.c (amd64_analyze_prologue): Merge op and buf. > --- > gdb/amd64-tdep.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index ff12cb874b8..c1a9b553e20 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch, > CORE_ADDR pc, CORE_ADDR current_pc, > struct amd64_frame_cache *cache) > { > - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > /* The `endbr64` instruction. */ > static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa }; > /* There are two variations of movq %rsp, %rbp. */ > @@ -2384,8 +2383,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch, > static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 }; > static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec }; > > - gdb_byte buf[3]; > - gdb_byte op; > + gdb_byte buf[4]; > > /* Analysis must not go past current_pc. */ > if (pc >= current_pc) > @@ -2396,24 +2394,20 @@ amd64_analyze_prologue (struct gdbarch *gdbarch, > else > pc = amd64_analyze_stack_align (pc, current_pc, cache); > > - op = read_code_unsigned_integer (pc, 1, byte_order); > - > - /* Check for the `endbr64` instruction, skip it if found. */ > - if (op == endbr64[0]) > + read_code (pc, buf, 4); > + /* Check for the `endbr64' instruction and skip it if found. */ > + if (memcmp (buf, endbr64, sizeof (endbr64)) == 0) > { > - read_code (pc + 1, buf, 3); > - > - if (memcmp (buf, &endbr64[1], 3) == 0) > - pc += 4; > + pc += sizeof (endbr64); > + /* If we went past the allowed bound, stop. */ > + if (pc >= current_pc) > + return current_pc; > > - op = read_code_unsigned_integer (pc, 1, byte_order); > + /* Update the code buffer, as pc changed. */ > + read_code (pc, buf, 1); > } > > - /* If we went past the allowed bound, stop. */ > - if (pc >= current_pc) > - return current_pc; > - > - if (op == 0x55) /* pushq %rbp */ > + if (buf[0] == 0x55) /* pushq %rbp */ > { > /* Take into account that we've executed the `pushq %rbp' that > starts this instruction sequence. */ If it's ok to read 4 bytes from the start, then why not merge remove the following read (not shown in the snippet): read_code (pc + 1, buf, 3); and make the read above (which happens if the endbr64 instruction is found) read 4 bytes directly? Simon