[v3,7/7] amd64_analyze_prologue: use target_read_code instead of read_code

Message ID 20200624012857.31849-8-vcollod@nvidia.com
State New
Headers show
Series
  • Improve intel IBT support
Related show

Commit Message

Mike Frysinger via Gdb-patches June 24, 2020, 1:28 a.m.
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(-)

-- 
2.20.1

Comments

Simon Marchi Aug. 6, 2020, 3:01 p.m. | #1
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

Patch

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