From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id +9EECSPI/F+YJwAAWB0awg (envelope-from ) for ; Mon, 11 Jan 2021 16:50:27 -0500 Received: by simark.ca (Postfix, from userid 112) id 151B71EEEF; Mon, 11 Jan 2021 16:50:27 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [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 D9AAF1E4F4 for ; Mon, 11 Jan 2021 16:50:25 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3AE24388A811; Mon, 11 Jan 2021 21:50:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3AE24388A811 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1610401825; bh=lK2npkJjjMfaWRZkpVERpie2RUSK0wMhraQVDRr3aKw=; 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=ALxmEnAwvEhQMUiuuSjPznlrkzgUBAfizv4xE9pFG8nTvMpvSj99thzeZvGQN2sLE JoQZ1Xi7OY21lysPV6l4+id6wJciAHAs1hngL18gAvwhFXaqLmI/h78eG2vsmp88O0 vaIb7X8OfaEkpmvGVtYFWlUcClZ5GqrVuuIq/kxo= Received: from mail-qk1-x735.google.com (mail-qk1-x735.google.com [IPv6:2607:f8b0:4864:20::735]) by sourceware.org (Postfix) with ESMTPS id 846A7386F43F for ; Mon, 11 Jan 2021 21:50:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 846A7386F43F Received: by mail-qk1-x735.google.com with SMTP id c7so276436qke.1 for ; Mon, 11 Jan 2021 13:50:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=lK2npkJjjMfaWRZkpVERpie2RUSK0wMhraQVDRr3aKw=; b=Dk7BGaQoYQRK7jl+rYqRz0ylGaYrWRkrrzcxrwiGF0++tsz3aMnTVFsBX5qgES0tQ1 xPOVhBcJhS2KoTBuYr3VmhYeYKodzSz0HCBNgQ7XMM+u4Dhv2RcMH/ctv6Yh5ZLUUKEV tMm2evsOK2WX5prsTRj+wI4KnB3gsw6RnwHMODCVjW4nJOpDJDprw2sFaCfundLdxy1w DMFNOMKPJDqqG4e9JgPCpyNlxqiacUJ5I+RggCIbQAuyjxxipJY8UcA2Niy/CCHQYXD5 4zymu63cCl+8Qqs4gCQrLA7Kw8Yqb2BNW2vWfQ2FrZqvmE9PYuCZIjeFC6+LvDkfdzCr 6AhQ== X-Gm-Message-State: AOAM530PSKGh1q2pbagsMz2uNBzdACengahrCxm/8cgTJBF6w03FK9Q+ Ww5uKxZ45WLEg5RY4DCI/8b3TNexu99zdw== X-Google-Smtp-Source: ABdhPJzoOtd+JjeLKelXOCKjpTH4mm5Eu1xqr9MGWEWD492AfrRGlF9kzHbpojrwLVmJAkYwiPAbHA== X-Received: by 2002:a37:8fc3:: with SMTP id r186mr1457070qkd.228.1610401821804; Mon, 11 Jan 2021 13:50:21 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:874d:20e9:a3d4:1db5:c30a? ([2804:7f0:8284:874d:20e9:a3d4:1db5:c30a]) by smtp.gmail.com with ESMTPSA id m10sm441012qtg.52.2021.01.11.13.50.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 Jan 2021 13:50:21 -0800 (PST) Subject: Re: [PATCH 3/5] gdb: convert aarch64 to new-style debug macros To: Simon Marchi References: <20210109042816.4140840-1-simon.marchi@polymtl.ca> <20210109042816.4140840-3-simon.marchi@polymtl.ca> <01831a09-012e-86ca-eee2-e8135f73fc21@polymtl.ca> Message-ID: Date: Mon, 11 Jan 2021 18:50:18 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <01831a09-012e-86ca-eee2-e8135f73fc21@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Luis Machado via Gdb-patches Reply-To: Luis Machado Cc: Gdb-patches Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi, On 1/11/21 6:30 PM, Simon Marchi wrote: > > > 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 :). This is not a big deal. Either way is a bit ugly to be honest. The problem lies on the limit we currently have. Maybe we can discuss that in the future. Meanwhile, your original patch is fine, so I'd go with it.