On Thu, Aug 6, 2020 at 6:59 AM Simon Marchi <simark@simark.ca> wrote:
>
> On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> > 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> >
>
> 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`.
>

I updated Victor's patch to fix:

https://sourceware.org/bugzilla/show_bug.cgi?id=26635

OK for master?

Thanks.

-- 
H.J.