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 8634E384B821 for ; Thu, 6 Aug 2020 13:57:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8634E384B821 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 CE5681E599; Thu, 6 Aug 2020 09:57:26 -0400 (EDT) Subject: Re: [PATCH v3 1/7] Add i386 support for endbr skipping To: Victor Collod , gdb-patches@sourceware.org References: <0cc93067-1313-6434-4330-61a21736376f@simark.ca> <20200624012857.31849-1-vcollod@nvidia.com> <20200624012857.31849-2-vcollod@nvidia.com> From: Simon Marchi Message-ID: <05f26b4d-5d2c-3dec-c129-61efc2ed7d1c@simark.ca> Date: Thu, 6 Aug 2020 09:57:18 -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-2-vcollod@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US 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 13:57:28 -0000 On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote: > 2020-06-11 Victor Collod > Please write a commit message that explains the change. Imagine that you are talking to somebody already somewhat knowledgeable about the subject matter, but who doesn't know what you are working on or the problem you are trying to fix (this will be the case for most people trying to understand this patch in the future, if they git-blame this code). So you don't to go in details explaining what prologue skipping is, for example, but you need to explain what triggered you to write this patch. What didn't work, what's the bug, what is the impact of the bug, how do you fix it? And since it's relevant to this patch, how do modify / improve the testsuite to make sure this gets tested? Since this was already explained in commit ac4a4f1cd7dc ("gdb: handle endbr64 instruction in amd64_analyze_prologue"), you can always refer to this commit and say that you are fixing the same bug, but for i386 instead of amd64. When referring to another commit, always include both the sha1 and the subject/title. The Linux kernel way of doing it [1] is fine. [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html > gdb/ChangeLog: > > * i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr. > (i386_analyze_prologue): Call i386_skip_endbr. > > gdb/testsuite/ChangeLog: > > * gdb.arch/amd64-prologue-skip-cf-protection.exp: Make the test > compatible with i386, and move it to... > * gdb.arch/i386-prologue-skip-cf-protection.exp: ... here. > * gdb.arch/amd64-prologue-skip-cf-protection.c: Move to... > * gdb.arch/i386-prologue-skip-cf-protection.c: ... here. > --- > gdb/i386-tdep.c | 19 +++++++++++++++++++ > ...n.c => i386-prologue-skip-cf-protection.c} | 0 > ...p => i386-prologue-skip-cf-protection.exp} | 2 +- > 3 files changed, 20 insertions(+), 1 deletion(-) > rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%) > rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%) > > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index 9b905c1996a..263a3fd452e 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c > @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] = > { 0 } > }; > > +/* Check whether PC points to an endbr32 instruction. */ > +static CORE_ADDR > +i386_skip_endbr (CORE_ADDR pc) > +{ > + static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb }; > + > + gdb_byte buf[sizeof (endbr32)]; > + > + /* Stop there if we can't read the code */ > + if (target_read_code (pc, buf, sizeof (endbr32))) Compare explicitly with `!= 0`. In the test, please update the comment on top of the file. Where it talks about the endbr64 instruction, it should now say: `endbr32` / `endbr64`. Simon