Re: [PATCH 3/5] gdb: convert aarch64 to new-style debug macros

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-11 8:18 a.m., Luis Machado wrote:
> Hi,
> 
> On 1/9/21 1:31 AM, Simon Marchi wrote:
>> Hi Luis,
>>
>> Would you mind giving this patch a quick look?
>>
>> Thanks!
>>
>> Simon
>>
>> On 2021-01-08 11:28 p.m., Simon Marchi wrote:
>>> I haven't tried this on an actual aarch64 machine, but I am able to
>>> exercise it like this:
>>>
>>>      (gdb) set debug aarch64
>>>      (gdb) maintenance selftest aa
>>>      Running selftest aarch64-analyze-prologue.
>>>      [aarch64] aarch64_analyze_prologue: prologue analysis gave up addr=0x14 opcode=0xf94013e0
>>>      Running selftest aarch64-process-record.
>>>      Ran 2 unit tests, 0 failed
>>>
>>> gdb/ChangeLog:
>>>
>>>     * arch/aarch64-insn.h (aarch64_debug_printf): New.
>>>     * arch/aarch64-insn.c: Use aarch64_debug_printf.
>>>     * aarch64-tdep.c: Use aarch64_debug_printf.
>>>
>>> Change-Id: Ifdb40e2816ab8e55a9aabb066d1833d9b5a46094
>>> ---
>>>   gdb/aarch64-tdep.c      | 89 ++++++++++++++++-------------------------
>>>   gdb/arch/aarch64-insn.c |  9 ++---
>>>   gdb/arch/aarch64-insn.h |  5 +++
>>>   3 files changed, 42 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 77e6ad700fcd..a51b3341e789 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -388,12 +388,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>>           regs[rd] = regs[rm];
>>>         else
>>>           {
>>> -          if (aarch64_debug)
>>> -        {
>>> -          debug_printf ("aarch64: prologue analysis gave up "
>>> -                "addr=%s opcode=0x%x (orr x register)\n",
>>> -                core_addr_to_string_nz (start), insn);
>>> -        }
>>> +          aarch64_debug_printf ("prologue analysis gave up "
>>> +                    "addr=%s opcode=0x%x (orr x register)",
>>> +                    core_addr_to_string_nz (start), insn);
>>> +
>>>             break;
>>>           }
>>>       }
>>> @@ -513,10 +511,9 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>>           }
>>>         else
>>>           {
>>> -          if (aarch64_debug)
>>> -        debug_printf ("aarch64: prologue analysis gave up addr=%s"
>>> -                  " opcode=0x%x (iclass)\n",
>>> -                  core_addr_to_string_nz (start), insn);
>>> +          aarch64_debug_printf ("prologue analysis gave up addr=%s"
>>> +                    " opcode=0x%x (iclass)",
>>> +                    core_addr_to_string_nz (start), insn);
>>>             break;
>>>           }
>>>   @@ -527,12 +524,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>>       }
>>>         else
>>>       {
>>> -      if (aarch64_debug)
>>> -        {
>>> -          debug_printf ("aarch64: prologue analysis gave up addr=%s"
>>> -                " opcode=0x%x\n",
>>> -                core_addr_to_string_nz (start), insn);
>>> -        }
>>> +      aarch64_debug_printf ("prologue analysis gave up addr=%s"
>>> +                " opcode=0x%x",
>>> +                core_addr_to_string_nz (start), insn);
>>> +
>>>         break;
>>>       }
>>>       }
>>> @@ -1606,12 +1601,10 @@ pass_in_x (struct gdbarch *gdbarch, struct regcache *regcache,
>>>         && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
>>>       regval <<= ((X_REGISTER_SIZE - partial_len) * TARGET_CHAR_BIT);
>>>   -      if (aarch64_debug)
>>> -    {
>>> -      debug_printf ("arg %d in %s = 0x%s\n", info->argnum,
>>> -            gdbarch_register_name (gdbarch, regnum),
>>> -            phex (regval, X_REGISTER_SIZE));
>>> -    }
>>> +      aarch64_debug_printf ("arg %d in %s = 0x%s", info->argnum,
>>> +                gdbarch_register_name (gdbarch, regnum),
>>> +                phex (regval, X_REGISTER_SIZE));
>>> +
>>>         regcache_cooked_write_unsigned (regcache, regnum, regval);
>>>         len -= partial_len;
>>>         buf += partial_len;
>>> @@ -1646,11 +1639,9 @@ pass_in_v (struct gdbarch *gdbarch,
>>>         memcpy (reg, buf, len);
>>>         regcache->cooked_write (regnum, reg);
>>>   -      if (aarch64_debug)
>>> -    {
>>> -      debug_printf ("arg %d in %s\n", info->argnum,
>>> -            gdbarch_register_name (gdbarch, regnum));
>>> -    }
>>> +      aarch64_debug_printf ("arg %d in %s", info->argnum,
>>> +                gdbarch_register_name (gdbarch, regnum));
>>> +
>>>         return 1;
>>>       }
>>>     info->nsrn = 8;
>>> @@ -1680,11 +1671,8 @@ pass_on_stack (struct aarch64_call_info *info, struct type *type,
>>>     if (align > 16)
>>>       align = 16;
>>>   -  if (aarch64_debug)
>>> -    {
>>> -      debug_printf ("arg %d len=%d @ sp + %d\n", info->argnum, len,
>>> -            info->nsaa);
>>> -    }
>>> +  aarch64_debug_printf ("arg %d len=%d @ sp + %d\n", info->argnum, len,
>>> +            info->nsaa);
>>>       item.len = len;
>>>     item.data = buf;
>>> @@ -1833,13 +1821,11 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>>>     /* The struct_return pointer occupies X8.  */
>>>     if (return_method != return_method_normal)
>>>       {
>>> -      if (aarch64_debug)
>>> -    {
>>> -      debug_printf ("struct return in %s = 0x%s\n",
>>> -            gdbarch_register_name (gdbarch,
>>> -                           AARCH64_STRUCT_RETURN_REGNUM),
>>> -            paddress (gdbarch, struct_addr));
>>> -    }
>>> +      aarch64_debug_printf
>>> +    ("struct return in %s = 0x%s",
>>> +     gdbarch_register_name (gdbarch, AARCH64_STRUCT_RETURN_REGNUM),
>>> +     paddress (gdbarch, struct_addr));
>>> +
> 
> Would it be possible to split this up differently, while still keeping the opening parenthesis on the same line as the call?

This doesn't work, the "AARCH64_STRUCT_RETURN_REGNUM" line is too long:

      aarch64_debug_printf ("struct return in %s = 0x%s",
			    gdbarch_register_name (gdbarch,
						   AARCH64_STRUCT_RETURN_REGNUM),
			    paddress (gdbarch, struct_addr));

but I could do:

      aarch64_debug_printf ("struct return in %s = 0x%s",
			    gdbarch_register_name
			      (gdbarch, AARCH64_STRUCT_RETURN_REGNUM),
			    paddress (gdbarch, struct_addr));

aarch64_debug_printf's parenthesis are on the same line, but gdbarch_register_name's
are not, it's a trade-off.

> 
>>>         regcache_cooked_write_unsigned (regcache, AARCH64_STRUCT_RETURN_REGNUM,
>>>                         struct_addr);
>>>       }
>>> @@ -2246,12 +2232,10 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>>>         gdb_byte buf[register_size (gdbarch, regno)];
>>>         gdb_assert (len <= sizeof (buf));
>>>   -      if (aarch64_debug)
>>> -        {
>>> -          debug_printf ("read HFA or HVA return value element %d from %s\n",
>>> -                i + 1,
>>> -                gdbarch_register_name (gdbarch, regno));
>>> -        }
>>> +      aarch64_debug_printf
>>> +        ("read HFA or HVA return value element %d from %s",
>>> +         i + 1, gdbarch_register_name (gdbarch, regno));
>>> +
> 
> Same here.
> 
> Otherwise, this looks OK. Thanks for converting it.

That would require splitting the string in two though, are you ok with that?

	  aarch64_debug_printf ("read HFA or HVA return value element %d "
				"from %s",
				i + 1, gdbarch_register_name (gdbarch, regno));

Splitting literal strings is higher on my list of things to try to avoid.  As
you saw, when space gets tight, I like to move the opening parenthesis to the
next line.  That makes it so the space we have for the arguments does not depend
on the length of the called function's name.  And since I like long, descriptive
function names, I use it often :).

Simon