gdb: remove casts to long in dwarf2/loc.c

Message ID 20231204204021.889415-1-simon.marchi@efficios.com
State New
Headers
Series gdb: remove casts to long in dwarf2/loc.c |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Simon Marchi Dec. 4, 2023, 8:40 p.m. UTC
  I spotted this while reviewing another patch, these casts to long are
unnecessary.  The result of subtracting two pointers is ptrdiff_t, which
can be printed with the `t` length specifier.

Change-Id: Idf8052b110a61007261be19137e4864ae6b37190
---
 gdb/dwarf2/loc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)


base-commit: 9d8fc40eb0e611162844eb7b89f1c76875153fbe
  

Comments

Tom de Vries Dec. 5, 2023, 8:06 a.m. UTC | #1
On 12/4/23 21:40, Simon Marchi wrote:
> I spotted this while reviewing another patch, these casts to long are
> unnecessary.  The result of subtracting two pointers is ptrdiff_t, which
> can be printed with the `t` length specifier.
> 

I'm not sure if it's relevant, but gdb_printf uses 
format_pieces::format_pieces, which supports a number of length 
specififers, but not 't'.

Thanks,
- Tom

> Change-Id: Idf8052b110a61007261be19137e4864ae6b37190
> ---
>   gdb/dwarf2/loc.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index 5b2d58ab44e4..606c97c745de 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -3329,10 +3329,9 @@ disassemble_dwarf_expression (struct ui_file *stream,
>         name = get_DW_OP_name (op);
>   
>         if (!name)
> -	error (_("Unrecognized DWARF opcode 0x%02x at %ld"),
> -	       op, (long) (data - 1 - start));
> -      gdb_printf (stream, "  %*ld: %s", indent + 4,
> -		  (long) (data - 1 - start), name);
> +	error (_("Unrecognized DWARF opcode 0x%02x at %td"), op,
> +	       data - 1 - start);
> +      gdb_printf (stream, "  %*td: %s", indent + 4, data - 1 - start, name);
>   
>         switch (op)
>   	{
> @@ -3515,15 +3514,13 @@ disassemble_dwarf_expression (struct ui_file *stream,
>   	case DW_OP_skip:
>   	  l = extract_signed_integer (data, 2, gdbarch_byte_order (arch));
>   	  data += 2;
> -	  gdb_printf (stream, " to %ld",
> -		      (long) (data + l - start));
> +	  gdb_printf (stream, " to %td", data + l - start);
>   	  break;
>   
>   	case DW_OP_bra:
>   	  l = extract_signed_integer (data, 2, gdbarch_byte_order (arch));
>   	  data += 2;
> -	  gdb_printf (stream, " %ld",
> -		      (long) (data + l - start));
> +	  gdb_printf (stream, " %td", data + l - start);
>   	  break;
>   
>   	case DW_OP_call2:
> 
> base-commit: 9d8fc40eb0e611162844eb7b89f1c76875153fbe
  
Simon Marchi Dec. 5, 2023, 4:11 p.m. UTC | #2
On 12/5/23 03:06, Tom de Vries wrote:
> On 12/4/23 21:40, Simon Marchi wrote:
>> I spotted this while reviewing another patch, these casts to long are
>> unnecessary.  The result of subtracting two pointers is ptrdiff_t, which
>> can be printed with the `t` length specifier.
>>
> 
> I'm not sure if it's relevant, but gdb_printf uses format_pieces::format_pieces, which supports a number of length specififers, but not 't'.

Shortly after sending the patch, I realized that (I only manually tested
a code path that uses the libc's printf, which worked).  I tried to send
this message, but it looks like I failed to reply all:

    Well, I didn't test this enough.  I tried %td with printf, but not with
    gdb_printf.  gdb_printf uses our home-grown implementation of printf,
    and it doesn't support fancy length specifiers.  So, forget about that.
    We could change the error() calls, but it just seems confusing if the
    calls next to it keep the cast.

Simon
  
Tom Tromey Dec. 5, 2023, 7:21 p.m. UTC | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon>     Well, I didn't test this enough.  I tried %td with printf, but not with
Simon>     gdb_printf.  gdb_printf uses our home-grown implementation of printf,
Simon>     and it doesn't support fancy length specifiers.

It's fine IMO if you want to add standard length specifiers.  Basically
the idea is to keep it compatible with the standards, so that gcc's
printf checking will still work.

Tom
  

Patch

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 5b2d58ab44e4..606c97c745de 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -3329,10 +3329,9 @@  disassemble_dwarf_expression (struct ui_file *stream,
       name = get_DW_OP_name (op);
 
       if (!name)
-	error (_("Unrecognized DWARF opcode 0x%02x at %ld"),
-	       op, (long) (data - 1 - start));
-      gdb_printf (stream, "  %*ld: %s", indent + 4,
-		  (long) (data - 1 - start), name);
+	error (_("Unrecognized DWARF opcode 0x%02x at %td"), op,
+	       data - 1 - start);
+      gdb_printf (stream, "  %*td: %s", indent + 4, data - 1 - start, name);
 
       switch (op)
 	{
@@ -3515,15 +3514,13 @@  disassemble_dwarf_expression (struct ui_file *stream,
 	case DW_OP_skip:
 	  l = extract_signed_integer (data, 2, gdbarch_byte_order (arch));
 	  data += 2;
-	  gdb_printf (stream, " to %ld",
-		      (long) (data + l - start));
+	  gdb_printf (stream, " to %td", data + l - start);
 	  break;
 
 	case DW_OP_bra:
 	  l = extract_signed_integer (data, 2, gdbarch_byte_order (arch));
 	  data += 2;
-	  gdb_printf (stream, " %ld",
-		      (long) (data + l - start));
+	  gdb_printf (stream, " %td", data + l - start);
 	  break;
 
 	case DW_OP_call2: