From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id z0D2LnTD/F/bJgAAWB0awg (envelope-from ) for ; Mon, 11 Jan 2021 16:30:28 -0500 Received: by simark.ca (Postfix, from userid 112) id B10C61EEEF; Mon, 11 Jan 2021 16:30:28 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 4B6021E4F4 for ; Mon, 11 Jan 2021 16:30:28 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E6EE83842436; Mon, 11 Jan 2021 21:30:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E6EE83842436 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1610400627; bh=TzulXnvGErxbYjKfV1PG43bMQuwmE2rUnON76JXyfyo=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=XmOJMKynJVySbfxpHKHZO2jpScWPHhp6k0ynilIR/sjVjg9OBF7tLHMjJBWqtd8Fj 3BHmshOqhcw8OLffhcfzIKe5v1GDFj5G6kmNCT1YycsKjArW3q8AEJsSed8HTbDG3p Gjz8SytjohAeTwybtGQcf7U4vhWG9IxQm1VnzaT0= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id DE858384242B for ; Mon, 11 Jan 2021 21:30:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DE858384242B Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 10BLUJjA025128 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 11 Jan 2021 16:30:24 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 10BLUJjA025128 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9FB491E4F4; Mon, 11 Jan 2021 16:30:19 -0500 (EST) Subject: Re: [PATCH 3/5] gdb: convert aarch64 to new-style debug macros To: Luis Machado References: <20210109042816.4140840-1-simon.marchi@polymtl.ca> <20210109042816.4140840-3-simon.marchi@polymtl.ca> Message-ID: <01831a09-012e-86ca-eee2-e8135f73fc21@polymtl.ca> Date: Mon, 11 Jan 2021 16:30:19 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 11 Jan 2021 21:30:19 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: Gdb-patches Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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