* [PATCH v3 1/7] Add i386 support for endbr skipping
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 13:57 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 2/7] amd64_analyze_prologue: swap upper bound check condition operands Victor Collod
` (6 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
2020-06-11 Victor Collod <vcollod@nvidia.com>
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)))
+ return pc;
+
+ /* If the instruction isn't an endbr32, stop */
+ if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
+ return pc;
+
+ return pc + sizeof (endbr32);
+}
/* Check whether PC points to a no-op instruction. */
static CORE_ADDR
@@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
CORE_ADDR pc, CORE_ADDR current_pc,
struct i386_frame_cache *cache)
{
+ pc = i386_skip_endbr (pc);
pc = i386_skip_noop (pc);
pc = i386_follow_jump (gdbarch, pc);
pc = i386_analyze_struct_return (pc, current_pc, cache);
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
similarity index 100%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
similarity index 97%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
index 3c51fd30352..612c6edf9f1 100644
--- a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
+++ b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
@@ -22,7 +22,7 @@
standard_testfile .c
set binfile ${binfile}
-if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
+if { ![istarget x86_64-*-*] && ![istarget i?86-*-*] } {
verbose "Skipping ${testfile}."
return
}
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/7] Add i386 support for endbr skipping
2020-06-24 1:28 ` [PATCH v3 1/7] Add i386 support for endbr skipping Victor Collod
@ 2020-08-06 13:57 ` Simon Marchi
2020-09-19 0:29 ` [PATCH] gdb: Update i386_analyze_prologue to skip endbr32 H.J. Lu
0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2020-08-06 13:57 UTC (permalink / raw)
To: Victor Collod, gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] gdb: Update i386_analyze_prologue to skip endbr32
2020-08-06 13:57 ` Simon Marchi
@ 2020-09-19 0:29 ` H.J. Lu
2020-09-19 0:38 ` Simon Marchi
0 siblings, 1 reply; 26+ messages in thread
From: H.J. Lu @ 2020-09-19 0:29 UTC (permalink / raw)
To: Simon Marchi; +Cc: Victor Collod, GDB
[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]
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.
[-- Attachment #2: 0001-gdb-Update-i386_analyze_prologue-to-skip-endbr32.patch --]
[-- Type: text/x-patch, Size: 4881 bytes --]
From 95a7715a0e085cbbaff33d9291647d5249a55b76 Mon Sep 17 00:00:00 2001
From: Victor Collod <vcollod@nvidia.com>
Date: Tue, 23 Jun 2020 18:28:51 -0700
Subject: [PATCH] gdb: Update i386_analyze_prologue to skip endbr32
With -m32 -fcf-protection, GCC generates an `endbr32` instruction at the
function entry:
[hjl@gnu-cfl-2 gdb]$ cat /tmp/x.c
int
main(void)
{
return 0;
}
[hjl@gnu-cfl-2 gdb]$ gcc -g -fcf-protection /tmp/x.c -m32
(gdb) b main
Breakpoint 1 at 0x8049176: file /tmp/x.c, line 3.
(gdb) r
Breakpoint 1, main () at /tmp/x.c:3
3 {
(gdb) disass
Dump of assembler code for function main:
=> 0x08049176 <+0>: endbr32
0x0804917a <+4>: push %ebp
0x0804917b <+5>: mov %esp,%ebp
0x0804917d <+7>: mov $0x0,%eax
0x08049182 <+12>: pop %ebp
0x08049183 <+13>: ret
End of assembler dump.
(gdb)
Update i386_analyze_prologue to skip `endbr32`:
(gdb) b main
Breakpoint 1 at 0x804917d: file /tmp/x.c, line 4.
(gdb) r
Breakpoint 1, main () at /tmp/x.c:4
4 return 0;
(gdb) disass
Dump of assembler code for function main:
0x08049176 <+0>: endbr32
0x0804917a <+4>: push %ebp
0x0804917b <+5>: mov %esp,%ebp
=> 0x0804917d <+7>: mov $0x0,%eax
0x08049182 <+12>: pop %ebp
0x08049183 <+13>: ret
End of assembler dump.
(gdb)
Tested with
$ make check RUNTESTFLAGS="--target_board='unix{-m32,}' i386-prologue-skip-cf-protection.exp"
on Fedora 32/x86-64.
2020-0X-YY Victor Collod <vcollod@nvidia.com>
gdb/ChangeLog:
PR gdb/26635
* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
(i386_analyze_prologue): Call i386_skip_endbr.
gdb/testsuite/ChangeLog:
PR gdb/26635
* 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} | 6 +++---
3 files changed, 22 insertions(+), 3 deletions(-)
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} (90%)
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 95cfe5b820e..b485f0b296a 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1538,6 +1538,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)))
+ return pc;
+
+ /* If the instruction isn't an endbr32, stop */
+ if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
+ return pc;
+
+ return pc + sizeof (endbr32);
+}
/* Check whether PC points to a no-op instruction. */
static CORE_ADDR
@@ -1815,6 +1833,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
CORE_ADDR pc, CORE_ADDR current_pc,
struct i386_frame_cache *cache)
{
+ pc = i386_skip_endbr (pc);
pc = i386_skip_noop (pc);
pc = i386_follow_jump (gdbarch, pc);
pc = i386_analyze_struct_return (pc, current_pc, cache);
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
similarity index 100%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
similarity index 90%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
index 3c51fd30352..9ba64f9c375 100644
--- a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
+++ b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
@@ -16,13 +16,13 @@
# Test skipping a prologue that was generated with gcc's -fcf-protection=full
# (control flow protection) option.
#
-# This option places an `endbr64` instruction at the start of all functions,
-# which can interfere with prologue analysis.
+# This option places an `endbr32`/`endbr64` instruction at the start of
+# all functions, which can interfere with prologue analysis.
standard_testfile .c
set binfile ${binfile}
-if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
+if { ![istarget x86_64-*-*] && ![istarget i?86-*-*] } {
verbose "Skipping ${testfile}."
return
}
--
2.26.2
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 2/7] amd64_analyze_prologue: swap upper bound check condition operands
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
2020-06-24 1:28 ` [PATCH v3 1/7] Add i386 support for endbr skipping Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 14:41 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf Victor Collod
` (5 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
`if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
and the test could be described as "stop if pc went past current_pc".
2020-06-23 Victor Collod <vcollod@nvidia.com>
* amd64-tdep.c (amd64_analyze_prologue): Swap upper bound check
condition operands.
---
gdb/amd64-tdep.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0ce9fbc2997..ff12cb874b8 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2387,7 +2387,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
gdb_byte buf[3];
gdb_byte op;
- if (current_pc <= pc)
+ /* Analysis must not go past current_pc. */
+ if (pc >= current_pc)
return current_pc;
if (gdbarch_ptr_bit (gdbarch) == 32)
@@ -2408,7 +2409,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
op = read_code_unsigned_integer (pc, 1, byte_order);
}
- if (current_pc <= pc)
+ /* If we went past the allowed bound, stop. */
+ if (pc >= current_pc)
return current_pc;
if (op == 0x55) /* pushq %rbp */
@@ -2418,8 +2420,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
cache->saved_regs[AMD64_RBP_REGNUM] = 0;
cache->sp_offset += 8;
- /* If that's all, return now. */
- if (current_pc <= pc + 1)
+ /* If we went past the allowed bound, stop. */
+ if (pc + 1 >= current_pc)
return current_pc;
read_code (pc + 1, buf, 3);
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
2020-06-24 1:28 ` [PATCH v3 1/7] Add i386 support for endbr skipping Victor Collod
2020-06-24 1:28 ` [PATCH v3 2/7] amd64_analyze_prologue: swap upper bound check condition operands Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 14:55 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 4/7] amd64_analyze_prologue: invert a condition for readability Victor Collod
` (4 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
Both variables were used to store function code.
2020-06-23 Victor Collod <vcollod@nvidia.com>
* 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. */
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf
2020-06-24 1:28 ` [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf Victor Collod
@ 2020-08-06 14:55 ` Simon Marchi
0 siblings, 0 replies; 26+ messages in thread
From: Simon Marchi @ 2020-08-06 14:55 UTC (permalink / raw)
To: Victor Collod, gdb-patches
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 <vcollod@nvidia.com>
>
> * 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 4/7] amd64_analyze_prologue: invert a condition for readability
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
` (2 preceding siblings ...)
2020-06-24 1:28 ` [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 14:57 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 5/7] amd64_analyze_prologue: gradually update pc Victor Collod
` (3 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
You can use git diff --ignore-space-at-eol -b -w --ignore-blank-lines
to make the patch clearer.
2020-06-23 Victor Collod <vcollod@nvidia.com>
* amd64-tdep.c (amd64_analyze_prologue): Invert a condition for readability.
---
gdb/amd64-tdep.c | 57 ++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index c1a9b553e20..17b02706e54 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2407,44 +2407,43 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
read_code (pc, buf, 1);
}
- if (buf[0] == 0x55) /* pushq %rbp */
- {
- /* Take into account that we've executed the `pushq %rbp' that
- starts this instruction sequence. */
- cache->saved_regs[AMD64_RBP_REGNUM] = 0;
- cache->sp_offset += 8;
+ /* Stop right now if there's no `pushq %rbp'. */
+ if (buf[0] != 0x55)
+ return pc;
- /* If we went past the allowed bound, stop. */
- if (pc + 1 >= current_pc)
- return current_pc;
+ /* Take into account that we've executed the `pushq %rbp' that
+ starts this instruction sequence. */
+ cache->saved_regs[AMD64_RBP_REGNUM] = 0;
+ cache->sp_offset += 8;
+
+ /* If we went past the allowed bound, stop. */
+ if (pc + 1 >= current_pc)
+ return current_pc;
+
+ read_code (pc + 1, buf, 3);
- read_code (pc + 1, buf, 3);
+ /* Check for `movq %rsp, %rbp'. */
+ if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
+ || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+ {
+ /* OK, we actually have a frame. */
+ cache->frameless_p = 0;
+ return pc + 4;
+ }
- /* Check for `movq %rsp, %rbp'. */
- if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
- || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+ /* For X32, also check for `movq %esp, %ebp'. */
+ if (gdbarch_ptr_bit (gdbarch) == 32)
+ {
+ if (memcmp (buf, mov_esp_ebp_1, 2) == 0
+ || memcmp (buf, mov_esp_ebp_2, 2) == 0)
{
/* OK, we actually have a frame. */
cache->frameless_p = 0;
- return pc + 4;
- }
-
- /* For X32, also check for `movq %esp, %ebp'. */
- if (gdbarch_ptr_bit (gdbarch) == 32)
- {
- if (memcmp (buf, mov_esp_ebp_1, 2) == 0
- || memcmp (buf, mov_esp_ebp_2, 2) == 0)
- {
- /* OK, we actually have a frame. */
- cache->frameless_p = 0;
- return pc + 3;
- }
+ return pc + 3;
}
-
- return pc + 1;
}
- return pc;
+ return pc + 1;
}
/* Work around false termination of prologue - GCC PR debug/48827.
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 5/7] amd64_analyze_prologue: gradually update pc
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
` (3 preceding siblings ...)
2020-06-24 1:28 ` [PATCH v3 4/7] amd64_analyze_prologue: invert a condition for readability Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 14:59 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment Victor Collod
` (2 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
It makes the function easier to read, as you don't have to remember
what's the current offset from pc.
2020-06-23 Victor Collod <vcollod@nvidia.com>
* amd64-tdep.c (amd64_analyze_prologue): Gradually update pc.
---
gdb/amd64-tdep.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 17b02706e54..5c3ad505784 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2416,19 +2416,22 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
cache->saved_regs[AMD64_RBP_REGNUM] = 0;
cache->sp_offset += 8;
+ pc += 1;
+
/* If we went past the allowed bound, stop. */
- if (pc + 1 >= current_pc)
+ if (pc >= current_pc)
return current_pc;
- read_code (pc + 1, buf, 3);
+ read_code (pc, buf, 3);
/* Check for `movq %rsp, %rbp'. */
if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
|| memcmp (buf, mov_rsp_rbp_2, 3) == 0)
{
+ pc += 3;
/* OK, we actually have a frame. */
cache->frameless_p = 0;
- return pc + 4;
+ return pc;
}
/* For X32, also check for `movq %esp, %ebp'. */
@@ -2437,13 +2440,14 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
if (memcmp (buf, mov_esp_ebp_1, 2) == 0
|| memcmp (buf, mov_esp_ebp_2, 2) == 0)
{
+ pc += 2;
/* OK, we actually have a frame. */
cache->frameless_p = 0;
- return pc + 3;
+ return pc;
}
}
- return pc + 1;
+ return pc;
}
/* Work around false termination of prologue - GCC PR debug/48827.
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
` (4 preceding siblings ...)
2020-06-24 1:28 ` [PATCH v3 5/7] amd64_analyze_prologue: gradually update pc Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 15:05 ` Simon Marchi
2020-06-24 1:28 ` [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code Victor Collod
2020-08-05 21:44 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
The width of the instruction didn't match the size of its operands.
2020-06-23 Victor Collod <vcollod@nvidia.com>
* amd64-tdep.c (amd64_analyze_prologue): Fix incorrect comment.
---
gdb/amd64-tdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 5c3ad505784..901733cf443 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2434,7 +2434,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
return pc;
}
- /* For X32, also check for `movq %esp, %ebp'. */
+ /* For X32, also check for `movl %esp, %ebp'. */
if (gdbarch_ptr_bit (gdbarch) == 32)
{
if (memcmp (buf, mov_esp_ebp_1, 2) == 0
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment
2020-06-24 1:28 ` [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment Victor Collod
@ 2020-08-06 15:05 ` Simon Marchi
0 siblings, 0 replies; 26+ messages in thread
From: Simon Marchi @ 2020-08-06 15:05 UTC (permalink / raw)
To: Victor Collod, gdb-patches
On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> The width of the instruction didn't match the size of its operands.
>
> 2020-06-23 Victor Collod <vcollod@nvidia.com>
>
> * amd64-tdep.c (amd64_analyze_prologue): Fix incorrect comment.
> ---
> gdb/amd64-tdep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 5c3ad505784..901733cf443 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2434,7 +2434,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> return pc;
> }
>
> - /* For X32, also check for `movq %esp, %ebp'. */
> + /* For X32, also check for `movl %esp, %ebp'. */
> if (gdbarch_ptr_bit (gdbarch) == 32)
> {
> if (memcmp (buf, mov_esp_ebp_1, 2) == 0
> --
> 2.20.1
>
Thanks, I pushed this fix right away since it's obvious.
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
` (5 preceding siblings ...)
2020-06-24 1:28 ` [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment Victor Collod
@ 2020-06-24 1:28 ` Victor Collod
2020-08-06 15:01 ` Simon Marchi
2020-08-05 21:44 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
7 siblings, 1 reply; 26+ messages in thread
From: Victor Collod @ 2020-06-24 1:28 UTC (permalink / raw)
To: gdb-patches
Using target_read_code enables gracefuly handling error cases.
2020-06-23 Victor Collod <vcollod@nvidia.com>
* amd64-tdep.c (amd64_analyze_prologue): Use target_read_code
instead of read_code.
---
gdb/amd64-tdep.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 901733cf443..7f70c1d0d8d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2394,9 +2394,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
else
pc = amd64_analyze_stack_align (pc, current_pc, cache);
- read_code (pc, buf, 4);
- /* Check for the `endbr64' instruction and skip it if found. */
- if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
+ /* Try to read enough bytes to check for `endbr64'. */
+ if (target_read_code (pc, buf, 4) != 0)
+ {
+ /* If it fails, read just enough data for `pushq %rbp'. */
+ if (target_read_code (pc, buf, 1) != 0)
+ return pc;
+ }
+ /* If reading succeeded, check for the `endbr64' instruction and skip it if found. */
+ else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
{
pc += sizeof (endbr64);
/* If we went past the allowed bound, stop. */
@@ -2404,7 +2410,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
return current_pc;
/* Update the code buffer, as pc changed. */
- read_code (pc, buf, 1);
+ if (target_read_code (pc, buf, 1) != 0)
+ return pc;
}
/* Stop right now if there's no `pushq %rbp'. */
@@ -2422,7 +2429,9 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
if (pc >= current_pc)
return current_pc;
- read_code (pc, buf, 3);
+ /* Try to read the code for the stack base move. */
+ if (target_read_code (pc, buf, 3) != 0)
+ return pc;
/* Check for `movq %rsp, %rbp'. */
if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code
2020-06-24 1:28 ` [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code Victor Collod
@ 2020-08-06 15:01 ` Simon Marchi
0 siblings, 0 replies; 26+ messages in thread
From: Simon Marchi @ 2020-08-06 15:01 UTC (permalink / raw)
To: Victor Collod, gdb-patches
On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> Using target_read_code enables gracefuly handling error cases.
Can you expand a bit why you think it's more graceful?
>
> 2020-06-23 Victor Collod <vcollod@nvidia.com>
>
> * amd64-tdep.c (amd64_analyze_prologue): Use target_read_code
> instead of read_code.
> ---
> gdb/amd64-tdep.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 901733cf443..7f70c1d0d8d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2394,9 +2394,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> else
> pc = amd64_analyze_stack_align (pc, current_pc, cache);
>
> - read_code (pc, buf, 4);
> - /* Check for the `endbr64' instruction and skip it if found. */
> - if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
> + /* Try to read enough bytes to check for `endbr64'. */
> + if (target_read_code (pc, buf, 4) != 0)
> + {
> + /* If it fails, read just enough data for `pushq %rbp'. */
> + if (target_read_code (pc, buf, 1) != 0)
> + return pc;
> + }
> + /* If reading succeeded, check for the `endbr64' instruction and skip it if found. */
> + else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
Personally, I find this "try to read 4 else try to read 1" approach less clear
and less intuitive than the "read 1 byte and read more if needed approach".
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/7] Improve intel IBT support
2020-06-24 1:28 ` [PATCH v3 0/7] Improve intel IBT support Victor Collod
` (6 preceding siblings ...)
2020-06-24 1:28 ` [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code Victor Collod
@ 2020-08-05 21:44 ` Victor Collod
7 siblings, 0 replies; 26+ messages in thread
From: Victor Collod @ 2020-08-05 21:44 UTC (permalink / raw)
To: gdb-patches
Hello,
Can someone have a look?
Thanks!
________________________________
From: Victor Collod <vcollod@nvidia.com>
Sent: Tuesday, June 23, 2020 6:28 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Victor Collod <vcollod@nvidia.com>
Subject: [PATCH v3 0/7] Improve intel IBT support
Thanks again for your review!
Victor Collod (7):
Add i386 support for endbr skipping
amd64_analyze_prologue: swap upper bound check condition operands
amd64_analyze_prologue: merge op and buf
amd64_analyze_prologue: invert a condition for readability
amd64_analyze_prologue: gradually update pc
amd64_analyze_prologue: fix incorrect comment
amd64_analyze_prologue: use target_read_code instead of read_code
gdb/amd64-tdep.c | 94 ++++++++++---------
gdb/i386-tdep.c | 19 ++++
...n.c => i386-prologue-skip-cf-protection.c} | 0
...p => i386-prologue-skip-cf-protection.exp} | 2 +-
4 files changed, 71 insertions(+), 44 deletions(-)
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%)
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread