Re: [PATCH v3 1/7] Add i386 support for endbr skipping

[Simon Marchi <simark@simark.ca>]

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`.

Simon