diff mbox series

[PATCHv3,2/3] gdb: move displaced_step_dump_bytes into gdbsupport (and rename)

Message ID a1e20b7203b0ed18edc784045c1999739f4513ba.1679919937.git.aburgess@redhat.com
State New
Headers
Series AMD64 Displaced Stepping Fix |

Commit Message

Andrew Burgess March 27, 2023, 12:32 p.m. UTC 
  It was pointed out during review of another patch that the function
displaced_step_dump_bytes really isn't specific to displaced stepping,
and should really get a more generic name and move into gdbsupport/.

This commit does just that.  The function is renamed to
bytes_to_string and is moved into gdbsupport/common-utils.{cc,h}.  The
function implementation doesn't really change. Much...

... I have updated the function to take an array view, which makes it
slightly easier to call in a couple of places where we already have a
gdb::bytes_vector.  I've then added an inline wrapper to convert a raw
pointer and length into an array view, which is used in places where
we don't easily have a gdb::bytes_vector (or similar).

Updated all users of displaced_step_dump_bytes.

There should be no user visible changes after this commit.
---
 gdb/amd64-tdep.c           |  2 +-
 gdb/displaced-stepping.c   |  3 +--
 gdb/i386-tdep.c            |  2 +-
 gdb/infrun.c               | 24 ++----------------------
 gdb/infrun.h               |  3 ---
 gdb/rs6000-tdep.c          |  2 +-
 gdb/s390-tdep.c            |  2 +-
 gdbsupport/array-view.h    |  1 +
 gdbsupport/common-utils.cc | 18 ++++++++++++++++++
 gdbsupport/common-utils.h  | 15 +++++++++++++++
 10 files changed, 41 insertions(+), 31 deletions(-)

Comments

Simon Marchi March 28, 2023, 1:10 p.m. UTC | #1 
> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
> index 3d8248b08b7..d07c8bc53fc 100644
> --- a/gdbsupport/array-view.h
> +++ b/gdbsupport/array-view.h
> @@ -21,6 +21,7 @@
>  #include "traits.h"
>  #include <algorithm>
>  #include <type_traits>
> +#include "gdbsupport/gdb_assert.h"
I suppose this is needed because array-view.h uses gdb_assert, and you
noticed it because some files didn't compile, because they didn't
happen to include gdb_assert.h before array-view.h.

> @@ -194,6 +195,20 @@ extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>  /* Like the above, but return a gdb::byte_vector.  */
>  gdb::byte_vector hex2bin (const char *hex);
>  
> +/* Build a string containing the contents of BYTES.  Each byte is
> +   represented as a 2 character hex string, with spaces separating each
> +   individual byte.  */
> +
> +extern std::string bytes_to_string (gdb::array_view<gdb_byte> bytes);

I think it could be const:

gdb::array_view<const gdb_byte>

And the other overload of bytes_to_string too.

Otherwise:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Andrew Burgess March 29, 2023, 9:43 a.m. UTC | #2 
Simon Marchi <simark@simark.ca> writes:

>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>> index 3d8248b08b7..d07c8bc53fc 100644
>> --- a/gdbsupport/array-view.h
>> +++ b/gdbsupport/array-view.h
>> @@ -21,6 +21,7 @@
>>  #include "traits.h"
>>  #include <algorithm>
>>  #include <type_traits>
>> +#include "gdbsupport/gdb_assert.h"
> I suppose this is needed because array-view.h uses gdb_assert, and you
> noticed it because some files didn't compile, because they didn't
> happen to include gdb_assert.h before array-view.h.
>
>> @@ -194,6 +195,20 @@ extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>>  /* Like the above, but return a gdb::byte_vector.  */
>>  gdb::byte_vector hex2bin (const char *hex);
>>  
>> +/* Build a string containing the contents of BYTES.  Each byte is
>> +   represented as a 2 character hex string, with spaces separating each
>> +   individual byte.  */
>> +
>> +extern std::string bytes_to_string (gdb::array_view<gdb_byte> bytes);
>
> I think it could be const:
>
> gdb::array_view<const gdb_byte>
>
> And the other overload of bytes_to_string too.
>
> Otherwise:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

I fixed the 'const' issue and pushed this patch.

Thanks,
Andrew
diff mbox series

Patch

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 81665e52d29..228b7518cb0 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1526,7 +1526,7 @@  amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   displaced_debug_printf ("copy %s->%s: %s",
 			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());
+			  bytes_to_string (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (dsc.release ());
diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 9f98ea8c35b..534e031a88e 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -122,8 +122,7 @@  displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
 
   displaced_debug_printf ("saved %s: %s",
 			  paddress (arch, buffer->addr),
-			  displaced_step_dump_bytes
-			  (buffer->saved_copy.data (), len).c_str ());
+			  bytes_to_string (buffer->saved_copy).c_str ());
 
   /* Save this in a local variable first, so it's released if code below
      throws.  */
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 96c04c1a3d6..e93479c35a3 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -830,7 +830,7 @@  i386_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   displaced_debug_printf ("%s->%s: %s",
 			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());
+			  bytes_to_string (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (closure.release ());
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8c56a9a4dfb..b021f40ae6e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1725,24 +1725,6 @@  displaced_step_reset (displaced_step_thread_state *displaced)
 
 using displaced_step_reset_cleanup = FORWARD_SCOPE_EXIT (displaced_step_reset);
 
-/* See infrun.h.  */
-
-std::string
-displaced_step_dump_bytes (const gdb_byte *buf, size_t len)
-{
-  std::string ret;
-
-  for (size_t i = 0; i < len; i++)
-    {
-      if (i == 0)
-	ret += string_printf ("%02x", buf[i]);
-      else
-	ret += string_printf (" %02x", buf[i]);
-    }
-
-  return ret;
-}
-
 /* Prepare to single-step, using displaced stepping.
 
    Note that we cannot use displaced stepping when we have a signal to
@@ -1820,8 +1802,7 @@  displaced_step_prepare_throw (thread_info *tp)
 	  gdb::byte_vector insn_buf (dislen);
 	  read_memory (original_pc, insn_buf.data (), insn_buf.size ());
 
-	  std::string insn_bytes
-	    = displaced_step_dump_bytes (insn_buf.data (), insn_buf.size ());
+	  std::string insn_bytes = bytes_to_string (insn_buf);
 
 	  displaced_debug_printf ("original insn %s: %s \t %s",
 				  paddress (gdbarch, original_pc),
@@ -1902,8 +1883,7 @@  displaced_step_prepare_throw (thread_info *tp)
 	  gdb::byte_vector insn_buf (dislen);
 	  read_memory (addr, insn_buf.data (), insn_buf.size ());
 
-	  std::string insn_bytes
-	    = displaced_step_dump_bytes (insn_buf.data (), insn_buf.size ());
+	  std::string insn_bytes = bytes_to_string (insn_buf);
 	  std::string insn_str = tmp_stream.release ();
 	  displaced_debug_printf ("replacement insn %s: %s \t %s",
 				  paddress (gdbarch, addr),
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 5219063586d..9b3c8962939 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -270,9 +270,6 @@  extern void update_signals_program_target (void);
    $_exitsignal.  */
 extern void clear_exit_convenience_vars (void);
 
-/* Dump LEN bytes at BUF in hex to a string and return it.  */
-extern std::string displaced_step_dump_bytes (const gdb_byte *buf, size_t len);
-
 extern void update_observer_mode (void);
 
 extern void signal_catch_update (const unsigned int *);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 52dcc89b2df..8e37c3d5183 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -940,7 +940,7 @@  ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   displaced_debug_printf ("copy %s->%s: %s",
 			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());
+			  bytes_to_string (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (closure.release ());
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index cab1757c5ab..081a8b68867 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -469,7 +469,7 @@  s390_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   displaced_debug_printf ("copy %s->%s: %s",
 			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());
+			  bytes_to_string (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (closure.release ());
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index 3d8248b08b7..d07c8bc53fc 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -21,6 +21,7 @@ 
 #include "traits.h"
 #include <algorithm>
 #include <type_traits>
+#include "gdbsupport/gdb_assert.h"
 
 /* An array_view is an abstraction that provides a non-owning view
    over a sequence of contiguous objects.
diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index e382fb28d8f..8e13d2f218b 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -445,3 +445,21 @@  hex2bin (const char *hex)
 
   return bin;
 }
+
+/* See gdbsupport/common-utils.h.  */
+
+std::string
+bytes_to_string (gdb::array_view<gdb_byte> bytes)
+{
+  std::string ret;
+
+  for (size_t i = 0; i < bytes.size (); i++)
+    {
+      if (i == 0)
+	ret += string_printf ("%02x", bytes[i]);
+      else
+	ret += string_printf (" %02x", bytes[i]);
+    }
+
+  return ret;
+}
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index 97dcb9fa8ce..8ee4549a43d 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -24,6 +24,7 @@ 
 #include <vector>
 #include "gdbsupport/byte-vector.h"
 #include "gdbsupport/gdb_unique_ptr.h"
+#include "gdbsupport/array-view.h"
 #include "poison.h"
 #include "gdb_string_view.h"
 
@@ -194,6 +195,20 @@  extern int hex2bin (const char *hex, gdb_byte *bin, int count);
 /* Like the above, but return a gdb::byte_vector.  */
 gdb::byte_vector hex2bin (const char *hex);
 
+/* Build a string containing the contents of BYTES.  Each byte is
+   represented as a 2 character hex string, with spaces separating each
+   individual byte.  */
+
+extern std::string bytes_to_string (gdb::array_view<gdb_byte> bytes);
+
+/* See bytes_to_string above.  This takes a BUFFER pointer and LENGTH
+   rather than an array view.  */
+
+static inline std::string bytes_to_string (gdb_byte *buffer, size_t length)
+{
+  return bytes_to_string ({buffer, length});
+}
+
 /* A fast hashing function.  This can be used to hash data in a fast way
    when the length is known.  If no fast hashing library is available, falls
    back to iterative_hash from libiberty.  START_VALUE can be set to