[PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom de Vries]

I noticed commit 84786372e1c ("Fix size of register buffer") fixing a
stack-buffer-overflow found by AddressSanitizer in
amd64_windows_store_arg_in_reg:
...
-  gdb_byte buf[8];
+  gdb_byte buf[16];
...
and wondered if we could have found this without AddressSanitizer.

I realized that the problem is that this:
...
  gdb_byte buf[N];
  ...
  regcache->cooked_write (regno, buf);
...
is using the deprecated variant of cooked_write instead of the one using
gdb::array_view:
...
  /* Transfer of pseudo-registers.  */
  void cooked_write (int regnum, gdb::array_view<const gdb_byte> src);

  /* Deprecated overload of the above.  */
  void cooked_write (int regnum, const gdb_byte *src);
...
and consequently cooked_write does not know the size of buf.

Fix this by using std::array, and likewise in other places in
gdb/amd64-windows-tdep.c.

In the process I fixed another out of bounds access here:
...
	gdb_byte imm16[2];
  ...
	cache->prev_sp = cur_sp
	  + extract_unsigned_integer (imm16, 4, byte_order);
...
where we're reading 4 bytes from the 2-byte buffer imm16.

Tested by rebuilding on x86_64-linux.
---
 gdb/amd64-windows-tdep.c | 46 ++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 57dcc4f56bd..b20c587c2f9 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -208,11 +208,11 @@ amd64_windows_store_arg_in_reg (struct regcache *regcache,
   struct type *type = arg->type ();
   const gdb_byte *valbuf = arg->contents ().data ();
   /* We only set 8 bytes, buf if it's a XMM register, 16 bytes are read.  */
-  gdb_byte buf[16];
+  std::array<gdb_byte, 16> buf;
 
   gdb_assert (type->length () <= 8);
-  memset (buf, 0, sizeof buf);
-  memcpy (buf, valbuf, std::min (type->length (), (ULONGEST) 8));
+  memset (buf.data (), 0, buf.size ());
+  memcpy (buf.data (), valbuf, std::min (type->length (), (ULONGEST) 8));
   regcache->cooked_write (regno, buf);
 }
 
@@ -317,7 +317,7 @@ amd64_windows_push_dummy_call
    function_call_return_method return_method, CORE_ADDR struct_addr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[8];
+  std::array<gdb_byte, 8> buf;
 
   /* Pass arguments.  */
   sp = amd64_windows_push_arguments (regcache, nargs, args, sp,
@@ -330,7 +330,7 @@ amd64_windows_push_dummy_call
 	 register.  */
       const int arg_regnum = amd64_windows_dummy_call_integer_regs[0];
 
-      store_unsigned_integer (buf, 8, byte_order, struct_addr);
+      store_unsigned_integer (buf, byte_order, struct_addr);
       regcache->cooked_write (arg_regnum, buf);
     }
 
@@ -340,11 +340,11 @@ amd64_windows_push_dummy_call
 
   /* Store return address.  */
   sp -= 8;
-  store_unsigned_integer (buf, 8, byte_order, bp_addr);
-  write_memory (sp, buf, 8);
+  store_unsigned_integer (buf, byte_order, bp_addr);
+  write_memory (sp, buf.data (), buf.size ());
 
   /* Update the stack pointer...  */
-  store_unsigned_integer (buf, 8, byte_order, sp);
+  store_unsigned_integer (buf, byte_order, sp);
   regcache->cooked_write (AMD64_RSP_REGNUM, buf);
 
   /* ...and fake a frame pointer.  */
@@ -433,13 +433,13 @@ amd64_skip_main_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   target_read_memory (pc, &op, 1);
   if (op == 0xe8)
     {
-      gdb_byte buf[4];
+      std::array<gdb_byte, 4> buf;
 
-      if (target_read_memory (pc + 1, buf, sizeof buf) == 0)
+      if (target_read_memory (pc + 1, buf.data (), buf.size ()) == 0)
 	{
 	  CORE_ADDR call_dest;
 
-	  call_dest = pc + 5 + extract_signed_integer (buf, 4, byte_order);
+	  call_dest = pc + 5 + extract_signed_integer (buf, byte_order);
 	  bound_minimal_symbol s = lookup_minimal_symbol_by_pc (call_dest);
 	  if (s.minsym != NULL
 	      && s.minsym->linkage_name () != NULL
@@ -617,12 +617,12 @@ amd64_windows_frame_decode_epilogue (const frame_info_ptr &this_frame,
     case 0xec:
       {
 	/* jmp rel32  */
-	gdb_byte rel32[4];
+	std::array<gdb_byte, 4> rel32;
 	CORE_ADDR npc;
 
-	if (target_read_memory (pc + 1, rel32, 4) != 0)
+	if (target_read_memory (pc + 1, rel32.data (), rel32.size ()) != 0)
 	  return -1;
-	npc = pc + 5 + extract_signed_integer (rel32, 4, byte_order);
+	npc = pc + 5 + extract_signed_integer (rel32, byte_order);
 
 	/* If the jump is within the function, then this is not a marker,
 	   otherwise this is a tail-call.  */
@@ -632,13 +632,13 @@ amd64_windows_frame_decode_epilogue (const frame_info_ptr &this_frame,
     case 0xc2:
       {
 	/* ret n  */
-	gdb_byte imm16[2];
+	std::array<gdb_byte, 2> imm16;
 
-	if (target_read_memory (pc + 1, imm16, 2) != 0)
+	if (target_read_memory (pc + 1, imm16.data (), imm16.size ()) != 0)
 	  return -1;
 	cache->prev_rip_addr = cur_sp;
 	cache->prev_sp = cur_sp
-	  + extract_unsigned_integer (imm16, 4, byte_order);
+	  + extract_unsigned_integer (imm16, byte_order);
 	return 1;
       }
 
@@ -797,11 +797,11 @@ amd64_windows_frame_decode_insns (const frame_info_ptr &this_frame,
 	  /* According to msdn:
 	     If an FP reg is used, then any unwind code taking an offset must
 	     only be used after the FP reg is established in the prolog.  */
-	  gdb_byte buf[8];
+	  std::array<gdb_byte, 8> buf;
 	  int frreg = amd64_windows_w2gdb_regnum[frame_reg];
 
-	  get_frame_register (this_frame, frreg, buf);
-	  save_addr = extract_unsigned_integer (buf, 8, byte_order);
+	  get_frame_register (this_frame, frreg, buf.data ());
+	  save_addr = extract_unsigned_integer (buf, byte_order);
 
 	  frame_debug_printf ("   frame_reg=%s, val=%s",
 			      gdbarch_register_name (gdbarch, frreg),
@@ -1084,7 +1084,7 @@ amd64_windows_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct amd64_windows_frame_cache *cache;
-  gdb_byte buf[8];
+  std::array<gdb_byte, 8> buf;
   CORE_ADDR pc;
   CORE_ADDR unwind_info = 0;
 
@@ -1096,8 +1096,8 @@ amd64_windows_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
 
   /* Get current PC and SP.  */
   pc = get_frame_pc (this_frame);
-  get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
-  cache->sp = extract_unsigned_integer (buf, 8, byte_order);
+  get_frame_register (this_frame, AMD64_RSP_REGNUM, buf.data ());
+  cache->sp = extract_unsigned_integer (buf, byte_order);
   cache->pc = pc;
 
   /* If we can't find the unwind info, keep trying as though this is a

base-commit: f4e363cae297ec3e24dec0d95f3d422879f498a3
-- 
2.35.3

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Hannes Domani]

 Am Dienstag, 29. Oktober 2024 um 12:50:12 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:

> I noticed commit 84786372e1c ("Fix size of register buffer") fixing a
> stack-buffer-overflow found by AddressSanitizer in
> amd64_windows_store_arg_in_reg:
> ...
> -  gdb_byte buf[8];
> +  gdb_byte buf[16];
> ...
> and wondered if we could have found this without AddressSanitizer.
>
> I realized that the problem is that this:
> ...
>   gdb_byte buf[N];
>   ...
>   regcache->cooked_write (regno, buf);
> ...
> is using the deprecated variant of cooked_write instead of the one using
> gdb::array_view:
> ...
>   /* Transfer of pseudo-registers.  */
>   void cooked_write (int regnum, gdb::array_view<const gdb_byte> src);
>
>   /* Deprecated overload of the above.  */
>   void cooked_write (int regnum, const gdb_byte *src);
> ...
> and consequently cooked_write does not know the size of buf.
>
> Fix this by using std::array, and likewise in other places in
> gdb/amd64-windows-tdep.c.
>
> In the process I fixed another out of bounds access here:
> ...
>     gdb_byte imm16[2];
>   ...
>     cache->prev_sp = cur_sp
>       + extract_unsigned_integer (imm16, 4, byte_order);
> ...
> where we're reading 4 bytes from the 2-byte buffer imm16.
>
> Tested by rebuilding on x86_64-linux.
> ---
> gdb/amd64-windows-tdep.c | 46 ++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
> index 57dcc4f56bd..b20c587c2f9 100644
> --- a/gdb/amd64-windows-tdep.c
> +++ b/gdb/amd64-windows-tdep.c
> @@ -208,11 +208,11 @@ amd64_windows_store_arg_in_reg (struct regcache *regcache,
>   struct type *type = arg->type ();
>   const gdb_byte *valbuf = arg->contents ().data ();
>   /* We only set 8 bytes, buf if it's a XMM register, 16 bytes are read.  */
> -  gdb_byte buf[16];
> +  std::array<gdb_byte, 16> buf;
>
>   gdb_assert (type->length () <= 8);
> -  memset (buf, 0, sizeof buf);
> -  memcpy (buf, valbuf, std::min (type->length (), (ULONGEST) 8));
> +  memset (buf.data (), 0, buf.size ());
> +  memcpy (buf.data (), valbuf, std::min (type->length (), (ULONGEST) 8));
>   regcache->cooked_write (regno, buf);
> }
>

The rest of the patch seems fine, but with this hunk applied, I get the
following result when trying to call any function with arguments:

(gdb) p (int)abs(-5)
C:/src/repos/binutils-gdb.git/gdb/regcache.c:864: internal-error: raw_write: Assertion `src.size () == m_descr->sizeof_register[regnum]' failed.
A problem internal to GDB has been detected,


(top-gdb) bt
#0  internal_error_loc (file=file@entry=0x140e592d8 <record_longname+5656> "C:/src/repos/binutils-gdb.git/gdb/regcache.c", line=line@entry=864, fmt=fmt@entry=0x140e5923e <record_longname+5502> "%s: Assertion `%s' failed.") at C:/src/repos/binutils-gdb.git/gdbsupport/errors.cc:53
#1  0x000000013feaf1ea in regcache::raw_write (this=0x4926c20, regnum=<optimized out>, src=array_view with 16 elements = {...}) at C:/src/repos/binutils-gdb.git/gdb/regcache.c:864
#2  0x000000013feb0869 in regcache::cooked_write (this=0x4926c20, regnum=2, src=array_view with 16 elements = {...}) at C:/src/repos/binutils-gdb.git/gdb/regcache.c:914
#3  0x000000013fb2ff0d in amd64_windows_store_arg_in_reg (regcache=0x4926c20, arg=<optimized out>, regno=2) at C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:217
#4  0x000000013fb30a12 in amd64_windows_push_arguments (regcache=regcache@entry=0x4926c20, nargs=nargs@entry=1, args=0x3ced30, args@entry=0x4c48a70, sp=sp@entry=0x3cf8a0, return_method=return_method@entry=return_method_normal) at C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:279
#5  0x000000013fb30d97 in amd64_windows_push_dummy_call (gdbarch=<optimized out>, function=<optimized out>, regcache=0x4926c20, bp_addr=0x3cf8af, nargs=1, args=0x4c48a70, sp=0x3cf8a0, return_method=return_method_normal, struct_addr=0x0) at C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:324
#6  0x000000013fd2f0a9 in call_function_by_hand_dummy (function=0x59e14d0, default_return_type=default_return_type@entry=0x4417670, args=..., dummy_dtor=dummy_dtor@entry=0x0, dummy_dtor_data=dummy_dtor_data@entry=0x0) at C:/src/repos/binutils-gdb.git/gdb/infcall.c:1463
#7  0x000000013fd31abb in call_function_by_hand (function=<optimized out>, default_return_type=default_return_type@entry=0x4417670, args=...) at C:/src/repos/binutils-gdb.git/gdb/infcall.c:1020
#8  0x000000013fcb556b in evaluate_subexp_do_call (exp=exp@entry=0x59cb480, noside=noside@entry=EVAL_NORMAL, callee=<optimized out>, callee@entry=0x59e14d0, argvec=array_view with 1 elements = {...}, function_name=function_name@entry=0x52d2840 "abs", default_return_type=default_return_type@entry=0x4417670) at C:/src/repos/binutils-gdb.git/gdb/eval.c:673
#9  0x000000013fcb5a24 in expr::operation::evaluate_funcall (this=this@entry=0x435faa0, expect_type=0x4417670, exp=0x59cb480, noside=EVAL_NORMAL, function_name=0x52d2840 "abs", args=std::vector of length 1, capacity 1 = {...}) at C:/src/repos/binutils-gdb.git/gdb/eval.c:705
#10 0x000000014095acc2 in expr::var_msym_value_operation::evaluate_funcall (this=0x435faa0, expect_type=<optimized out>, exp=<optimized out>, noside=<optimized out>, args=std::vector of length 1, capacity 1 = {...}) at C:/src/repos/binutils-gdb.git/gdb/expop.h:746
#11 0x00000001409563cb in expr::funcall_operation::evaluate (this=<optimized out>, expect_type=<optimized out>, exp=<optimized out>, noside=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/expop.h:2207
#12 0x000000013fcb093e in expr::operation::evaluate_for_cast (this=<optimized out>, expect_type=0x4417670, exp=<optimized out>, noside=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/eval.c:2626
#13 0x000000013fcb1892 in expression::evaluate (this=0x59cb480, expect_type=expect_type@entry=0x0, noside=noside@entry=EVAL_NORMAL) at C:/src/repos/binutils-gdb.git/gdb/eval.c:110
#14 0x000000013fe48946 in process_print_command_args (args=<optimized out>, print_opts=print_opts@entry=0x3cf7d0, voidprint=voidprint@entry=true) at C:/src/repos/binutils-gdb.git/gdb/printcmd.c:1327
#15 0x000000013fe49211 in print_command_1 (args=<optimized out>, voidprint=1) at C:/src/repos/binutils-gdb.git/gdb/printcmd.c:1340
#16 0x000000013fbe46a6 in cmd_func (cmd=<optimized out>, args=<optimized out>, from_tty=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/cli/cli-decode.c:2741
#17 0x000000013ffefaa5 in execute_command (p=<optimized out>, p@entry=0x59cb220 "p (int)abs(-5)", from_tty=1) at C:/src/repos/binutils-gdb.git/gdb/top.c:581
#18 0x000000013fcb7c9f in command_handler (command=0x59cb220 "p (int)abs(-5)") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:579
#19 0x000000013fcb9625 in command_line_handler (rl=...) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:815
#20 0x000000013fcb86d1 in gdb_rl_callback_handler (rl=0x59cb3c0 "p (int)abs(-5)") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:271
#21 0x000000014008af0f in rl_callback_read_char () at C:/src/repos/binutils-gdb.git/readline/readline/callback.c:290
#22 0x000000013fcb885e in gdb_rl_callback_read_char_wrapper_noexcept () at C:/src/repos/binutils-gdb.git/gdb/event-top.c:196
#23 0x000000013fcb8982 in gdb_rl_callback_read_char_wrapper (client_data=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:235
#24 0x00000001400250ac in stdin_event_handler (error=<optimized out>, client_data=0x4653b0) at C:/src/repos/binutils-gdb.git/gdb/ui.c:154
#25 0x0000000140569045 in handle_file_event (file_ptr=<optimized out>, ready_mask=<optimized out>) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:551
#26 gdb_wait_for_event (block=block@entry=1) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:694
#27 0x00000001405699d4 in gdb_wait_for_event (block=1) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:571
#28 gdb_do_one_event (mstimeout=mstimeout@entry=-1) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:263
#29 0x000000013fda02ea in start_event_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:404
#30 captured_command_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:468
#31 0x000000013fda4b35 in captured_main (data=0x3cfdd0) at C:/src/repos/binutils-gdb.git/gdb/main.c:1361
#32 gdb_main (args=args@entry=0x3cfe40) at C:/src/repos/binutils-gdb.git/gdb/main.c:1380
#33 0x0000000140a7c530 in main (argc=3, argv=0x437f20) at C:/src/repos/binutils-gdb.git/gdb/gdb.c:38


It's because raw_write expects an array_view with the exact size of the register:

regcache::raw_write (int regnum, gdb::array_view<const gdb_byte> src)
{
  assert_regnum (regnum);
  gdb_assert (src.size () == m_descr->sizeof_register[regnum]);


Hannes

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[tdevries]

On 2024-10-29 19:08, Hannes Domani wrote:
> Am Dienstag, 29. Oktober 2024 um 12:50:12 MEZ hat Tom de Vries
> <tdevries@suse.de> Folgendes geschrieben:
> 
>> I noticed commit 84786372e1c ("Fix size of register buffer") fixing a
>> stack-buffer-overflow found by AddressSanitizer in
>> amd64_windows_store_arg_in_reg:
>> ...
>> -  gdb_byte buf[8];
>> +  gdb_byte buf[16];
>> ...
>> and wondered if we could have found this without AddressSanitizer.
>> 
>> I realized that the problem is that this:
>> ...
>>    gdb_byte buf[N];
>>    ...
>>    regcache->cooked_write (regno, buf);
>> ...
>> is using the deprecated variant of cooked_write instead of the one 
>> using
>> gdb::array_view:
>> ...
>>    /* Transfer of pseudo-registers.  */
>>    void cooked_write (int regnum, gdb::array_view<const gdb_byte> 
>> src);
>> 
>>    /* Deprecated overload of the above.  */
>>    void cooked_write (int regnum, const gdb_byte *src);
>> ...
>> and consequently cooked_write does not know the size of buf.
>> 
>> Fix this by using std::array, and likewise in other places in
>> gdb/amd64-windows-tdep.c.
>> 
>> In the process I fixed another out of bounds access here:
>> ...
>>      gdb_byte imm16[2];
>>    ...
>>      cache->prev_sp = cur_sp
>>        + extract_unsigned_integer (imm16, 4, byte_order);
>> ...
>> where we're reading 4 bytes from the 2-byte buffer imm16.
>> 
>> Tested by rebuilding on x86_64-linux.
>> ---
>> gdb/amd64-windows-tdep.c | 46 ++++++++++++++++++++--------------------
>> 1 file changed, 23 insertions(+), 23 deletions(-)
>> 
>> diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
>> index 57dcc4f56bd..b20c587c2f9 100644
>> --- a/gdb/amd64-windows-tdep.c
>> +++ b/gdb/amd64-windows-tdep.c
>> @@ -208,11 +208,11 @@ amd64_windows_store_arg_in_reg (struct regcache 
>> *regcache,
>>    struct type *type = arg->type ();
>>    const gdb_byte *valbuf = arg->contents ().data ();
>>    /* We only set 8 bytes, buf if it's a XMM register, 16 bytes are 
>> read.  */
>> -  gdb_byte buf[16];
>> +  std::array<gdb_byte, 16> buf;
>> 
>>    gdb_assert (type->length () <= 8);
>> -  memset (buf, 0, sizeof buf);
>> -  memcpy (buf, valbuf, std::min (type->length (), (ULONGEST) 8));
>> +  memset (buf.data (), 0, buf.size ());
>> +  memcpy (buf.data (), valbuf, std::min (type->length (), (ULONGEST) 
>> 8));
>>    regcache->cooked_write (regno, buf);
>> }
>> 
> 
> The rest of the patch seems fine, but with this hunk applied, I get the
> following result when trying to call any function with arguments:
> 
> (gdb) p (int)abs(-5)
> C:/src/repos/binutils-gdb.git/gdb/regcache.c:864: internal-error:
> raw_write: Assertion `src.size () == m_descr->sizeof_register[regnum]'
> failed.
> A problem internal to GDB has been detected,
> 
> 
> (top-gdb) bt
> #0  internal_error_loc (file=file@entry=0x140e592d8
> <record_longname+5656> "C:/src/repos/binutils-gdb.git/gdb/regcache.c",
> line=line@entry=864, fmt=fmt@entry=0x140e5923e <record_longname+5502>
> "%s: Assertion `%s' failed.") at
> C:/src/repos/binutils-gdb.git/gdbsupport/errors.cc:53
> #1  0x000000013feaf1ea in regcache::raw_write (this=0x4926c20,
> regnum=<optimized out>, src=array_view with 16 elements = {...}) at
> C:/src/repos/binutils-gdb.git/gdb/regcache.c:864
> #2  0x000000013feb0869 in regcache::cooked_write (this=0x4926c20,
> regnum=2, src=array_view with 16 elements = {...}) at
> C:/src/repos/binutils-gdb.git/gdb/regcache.c:914
> #3  0x000000013fb2ff0d in amd64_windows_store_arg_in_reg
> (regcache=0x4926c20, arg=<optimized out>, regno=2) at
> C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:217
> #4  0x000000013fb30a12 in amd64_windows_push_arguments
> (regcache=regcache@entry=0x4926c20, nargs=nargs@entry=1,
> args=0x3ced30, args@entry=0x4c48a70, sp=sp@entry=0x3cf8a0,
> return_method=return_method@entry=return_method_normal) at
> C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:279
> #5  0x000000013fb30d97 in amd64_windows_push_dummy_call
> (gdbarch=<optimized out>, function=<optimized out>,
> regcache=0x4926c20, bp_addr=0x3cf8af, nargs=1, args=0x4c48a70,
> sp=0x3cf8a0, return_method=return_method_normal, struct_addr=0x0) at
> C:/src/repos/binutils-gdb.git/gdb/amd64-windows-tdep.c:324
> #6  0x000000013fd2f0a9 in call_function_by_hand_dummy
> (function=0x59e14d0,
> default_return_type=default_return_type@entry=0x4417670, args=...,
> dummy_dtor=dummy_dtor@entry=0x0,
> dummy_dtor_data=dummy_dtor_data@entry=0x0) at
> C:/src/repos/binutils-gdb.git/gdb/infcall.c:1463
> #7  0x000000013fd31abb in call_function_by_hand (function=<optimized
> out>, default_return_type=default_return_type@entry=0x4417670,
> args=...) at C:/src/repos/binutils-gdb.git/gdb/infcall.c:1020
> #8  0x000000013fcb556b in evaluate_subexp_do_call
> (exp=exp@entry=0x59cb480, noside=noside@entry=EVAL_NORMAL,
> callee=<optimized out>, callee@entry=0x59e14d0, argvec=array_view with
> 1 elements = {...}, function_name=function_name@entry=0x52d2840 "abs",
> default_return_type=default_return_type@entry=0x4417670) at
> C:/src/repos/binutils-gdb.git/gdb/eval.c:673
> #9  0x000000013fcb5a24 in expr::operation::evaluate_funcall
> (this=this@entry=0x435faa0, expect_type=0x4417670, exp=0x59cb480,
> noside=EVAL_NORMAL, function_name=0x52d2840 "abs", args=std::vector of
> length 1, capacity 1 = {...}) at
> C:/src/repos/binutils-gdb.git/gdb/eval.c:705
> #10 0x000000014095acc2 in
> expr::var_msym_value_operation::evaluate_funcall (this=0x435faa0,
> expect_type=<optimized out>, exp=<optimized out>, noside=<optimized
> out>, args=std::vector of length 1, capacity 1 = {...}) at
> C:/src/repos/binutils-gdb.git/gdb/expop.h:746
> #11 0x00000001409563cb in expr::funcall_operation::evaluate
> (this=<optimized out>, expect_type=<optimized out>, exp=<optimized
> out>, noside=<optimized out>) at
> C:/src/repos/binutils-gdb.git/gdb/expop.h:2207
> #12 0x000000013fcb093e in expr::operation::evaluate_for_cast
> (this=<optimized out>, expect_type=0x4417670, exp=<optimized out>,
> noside=<optimized out>) at
> C:/src/repos/binutils-gdb.git/gdb/eval.c:2626
> #13 0x000000013fcb1892 in expression::evaluate (this=0x59cb480,
> expect_type=expect_type@entry=0x0, noside=noside@entry=EVAL_NORMAL) at
> C:/src/repos/binutils-gdb.git/gdb/eval.c:110
> #14 0x000000013fe48946 in process_print_command_args (args=<optimized
> out>, print_opts=print_opts@entry=0x3cf7d0,
> voidprint=voidprint@entry=true) at
> C:/src/repos/binutils-gdb.git/gdb/printcmd.c:1327
> #15 0x000000013fe49211 in print_command_1 (args=<optimized out>,
> voidprint=1) at C:/src/repos/binutils-gdb.git/gdb/printcmd.c:1340
> #16 0x000000013fbe46a6 in cmd_func (cmd=<optimized out>,
> args=<optimized out>, from_tty=<optimized out>) at
> C:/src/repos/binutils-gdb.git/gdb/cli/cli-decode.c:2741
> #17 0x000000013ffefaa5 in execute_command (p=<optimized out>,
> p@entry=0x59cb220 "p (int)abs(-5)", from_tty=1) at
> C:/src/repos/binutils-gdb.git/gdb/top.c:581
> #18 0x000000013fcb7c9f in command_handler (command=0x59cb220 "p
> (int)abs(-5)") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:579
> #19 0x000000013fcb9625 in command_line_handler (rl=...) at
> C:/src/repos/binutils-gdb.git/gdb/event-top.c:815
> #20 0x000000013fcb86d1 in gdb_rl_callback_handler (rl=0x59cb3c0 "p
> (int)abs(-5)") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:271
> #21 0x000000014008af0f in rl_callback_read_char () at
> C:/src/repos/binutils-gdb.git/readline/readline/callback.c:290
> #22 0x000000013fcb885e in gdb_rl_callback_read_char_wrapper_noexcept
> () at C:/src/repos/binutils-gdb.git/gdb/event-top.c:196
> #23 0x000000013fcb8982 in gdb_rl_callback_read_char_wrapper
> (client_data=<optimized out>) at
> C:/src/repos/binutils-gdb.git/gdb/event-top.c:235
> #24 0x00000001400250ac in stdin_event_handler (error=<optimized out>,
> client_data=0x4653b0) at C:/src/repos/binutils-gdb.git/gdb/ui.c:154
> #25 0x0000000140569045 in handle_file_event (file_ptr=<optimized out>,
> ready_mask=<optimized out>) at
> C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:551
> #26 gdb_wait_for_event (block=block@entry=1) at
> C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:694
> #27 0x00000001405699d4 in gdb_wait_for_event (block=1) at
> C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:571
> #28 gdb_do_one_event (mstimeout=mstimeout@entry=-1) at
> C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:263
> #29 0x000000013fda02ea in start_event_loop () at
> C:/src/repos/binutils-gdb.git/gdb/main.c:404
> #30 captured_command_loop () at 
> C:/src/repos/binutils-gdb.git/gdb/main.c:468
> #31 0x000000013fda4b35 in captured_main (data=0x3cfdd0) at
> C:/src/repos/binutils-gdb.git/gdb/main.c:1361
> #32 gdb_main (args=args@entry=0x3cfe40) at
> C:/src/repos/binutils-gdb.git/gdb/main.c:1380
> #33 0x0000000140a7c530 in main (argc=3, argv=0x437f20) at
> C:/src/repos/binutils-gdb.git/gdb/gdb.c:38
> 
> 
> It's because raw_write expects an array_view with the exact size of
> the register:
> 
> regcache::raw_write (int regnum, gdb::array_view<const gdb_byte> src)
> {
>   assert_regnum (regnum);
>   gdb_assert (src.size () == m_descr->sizeof_register[regnum]);
> 
> 

Hi Hannes,

thanks for testing.

I've submitted a v2 that hopefully should take of this.

Thanks,
- Tom

> Hannes

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +  std::array<gdb_byte, 16> buf;
 
Tom>    gdb_assert (type->length () <= 8);
Tom> -  memset (buf, 0, sizeof buf);
Tom> -  memcpy (buf, valbuf, std::min (type->length (), (ULONGEST) 8));
Tom> +  memset (buf.data (), 0, buf.size ());
Tom> +  memcpy (buf.data (), valbuf, std::min (type->length (), (ULONGEST) 8));

Maybe buf can be value-initialized rather than using memset,
like 'std::array<..> buf {}'

Tom> +  write_memory (sp, buf.data (), buf.size ());

Tom> +      if (target_read_memory (pc + 1, buf.data (), buf.size ()) == 0)

I guess at some point we could have some more overloads accepting an
array_view.

Tom

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom de Vries]

On 11/1/24 15:25, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +  std::array<gdb_byte, 16> buf;
>   
> Tom>    gdb_assert (type->length () <= 8);
> Tom> -  memset (buf, 0, sizeof buf);
> Tom> -  memcpy (buf, valbuf, std::min (type->length (), (ULONGEST) 8));
> Tom> +  memset (buf.data (), 0, buf.size ());
> Tom> +  memcpy (buf.data (), valbuf, std::min (type->length (), (ULONGEST) 8));
> 
> Maybe buf can be value-initialized rather than using memset,
> like 'std::array<..> buf {}'
> 

Fixed in a follow-up patch ( 
https://sourceware.org/pipermail/gdb-patches/2024-November/212848.html 
), which also addresses your review comments from the other reply.

> Tom> +  write_memory (sp, buf.data (), buf.size ());
> 
> Tom> +      if (target_read_memory (pc + 1, buf.data (), buf.size ()) == 0)
> 
> I guess at some point we could have some more overloads accepting an
> array_view.

I had the same thought.

I also looked at how much the deprecated regcache functions were used, 
which is a lot.

I guess calling it deprecated in a comment doesn't do much to make the 
problem visible.

Marking them with [[deprecated]] makes the -Werror build fail (which is 
my default build setting), but I suppose I can just start using 
-Wno-error=deprecated-declarations.  Though, that will make newly 
introduced uses of deprecated functions less visible.

I played around with:
...
#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wdeprecated-declarations"
[[deprecated]]
    ...
#pragma GCC diagnostic pop
...
but that has no effect on the declaration, only on the uses.

Renaming the deprecated cooked_write to deprecated_cooked_write would 
work, but that also looks like a fair amount of work. Maybe some 
refactoring script to translate build errors into fixes would work.

Thanks,
- Tom

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I guess calling it deprecated in a comment doesn't do much to make the
Tom> problem visible.

It was before my time but I gather there was a wave of "deprecation"
in the old days that consisted of just renaming a bunch of stuff.
Nobody ever went and did the work of really updating the uses.

It's kind of hard to fix this since testing every target is difficult,
and many of them aren't really maintained.  So we just leave things like
they are and try not to let new deprecated uses enter the tree.

It's not even totally clear that all the deprecations make sense.  Like
for instance deprecated_safe_get_selected_frame seems handy to me.

Anyway, I support movement in this area.  It's just difficult.

Tom

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom de Vries]

On 11/3/24 17:58, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> I guess calling it deprecated in a comment doesn't do much to make the
> Tom> problem visible.
> 
> It was before my time but I gather there was a wave of "deprecation"
> in the old days that consisted of just renaming a bunch of stuff.
> Nobody ever went and did the work of really updating the uses.
> 
> It's kind of hard to fix this since testing every target is difficult,
> and many of them aren't really maintained.  So we just leave things like
> they are and try not to let new deprecated uses enter the tree.

That's understandable.  At least having things tagged with deprecated in 
the name makes sure the status quo doesn't get worse accidentally.

I tried the renaming scheme using error-driven renaming, and that sort 
of works, only I ran into a case that I didn't expect.

In summary:
- you have two variants of f, one deprecated and one not
- you rename the deprecated variant to to deprecated_f
- there are compiler errors that you handle by renaming the call to
   f to deprecated_f
- however, there are cases where what used to be a call to the
   deprecated variant is now a call to the non-deprecated variant, and
   due to stricter checking, this causes assertion failures.

In order to find those cases, you need to actually test the target, 
which as you mention is difficult, so I'm not yet sure this approach is 
a good idea.   There's the possibility to reintroduce the deprecated 
function under name f, but with the "[[deprecated]]" tag, which should 
ensure that functionality stays the same without having to test each 
target.  I'm currently considering this approach.

Anyway, the first such case I ran into was a case of raw_supply with a 
zero-initialized buffer, so I wrote this series ( 
https://sourceware.org/pipermail/gdb-patches/2024-November/212864.html ).

> It's not even totally clear that all the deprecations make sense.  Like
> for instance deprecated_safe_get_selected_frame seems handy to me.
> 

Hmm, that one has been deprecated since 2003, that's funny.  You could 
submit a patch to undeprecate this one then.  Not my area of expertise 
though.

> Anyway, I support movement in this area.  It's just difficult.

Agreed, it's difficult.

Thanks,
- Tom

Re: [PATCH] [gdb/tdep] Use std::array in amd64-windows-tdep.c

[Tom de Vries]

On 11/4/24 17:54, Tom de Vries wrote:
> On 11/3/24 17:58, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> I guess calling it deprecated in a comment doesn't do much to 
>> make the
>> Tom> problem visible.
>>
>> It was before my time but I gather there was a wave of "deprecation"
>> in the old days that consisted of just renaming a bunch of stuff.
>> Nobody ever went and did the work of really updating the uses.
>>
>> It's kind of hard to fix this since testing every target is difficult,
>> and many of them aren't really maintained.  So we just leave things like
>> they are and try not to let new deprecated uses enter the tree.
> 
> That's understandable.  At least having things tagged with deprecated in 
> the name makes sure the status quo doesn't get worse accidentally.
> 
> I tried the renaming scheme using error-driven renaming, and that sort 
> of works, only I ran into a case that I didn't expect.
> 
> In summary:
> - you have two variants of f, one deprecated and one not
> - you rename the deprecated variant to to deprecated_f
> - there are compiler errors that you handle by renaming the call to
>    f to deprecated_f
> - however, there are cases where what used to be a call to the
>    deprecated variant is now a call to the non-deprecated variant, and
>    due to stricter checking, this causes assertion failures.
> 
> In order to find those cases, you need to actually test the target, 
> which as you mention is difficult, so I'm not yet sure this approach is 
> a good idea.   There's the possibility to reintroduce the deprecated 
> function under name f, but with the "[[deprecated]]" tag, which should 
> ensure that functionality stays the same without having to test each 
> target.  I'm currently considering this approach.
> 

Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-November/213083.html ).

Thanks,
- Tom

> Anyway, the first such case I ran into was a case of raw_supply with a 
> zero-initialized buffer, so I wrote this series ( https:// 
> sourceware.org/pipermail/gdb-patches/2024-November/212864.html ).
> 
>> It's not even totally clear that all the deprecations make sense.  Like
>> for instance deprecated_safe_get_selected_frame seems handy to me.
>>
> 
> Hmm, that one has been deprecated since 2003, that's funny.  You could 
> submit a patch to undeprecate this one then.  Not my area of expertise 
> though.
> 
>> Anyway, I support movement in this area.  It's just difficult.
> 
> Agreed, it's difficult.
> 
> Thanks,
> - Tom
>