From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id jc2/JvE04GFoPQAAWB0awg (envelope-from ) for ; Thu, 13 Jan 2022 09:19:29 -0500 Received: by simark.ca (Postfix, from userid 112) id 81C521F34E; Thu, 13 Jan 2022 09:19:29 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,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 9C4091EA69 for ; Thu, 13 Jan 2022 09:19:28 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0A3BD3951C1E for ; Thu, 13 Jan 2022 14:19:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0A3BD3951C1E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1642083568; bh=LJeaNxfsZB8KGTmysyMM3Nq2dvWxfDyeTzRVV/Tx5to=; 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=UF1JlxaZKQZldPmySXe1pfTY9ExdYYmpMUvgfFphEpY/bygtVK18sXIJJUzh4qIjZ g1Zt42Q0J3b0njF3fcZzJwv5C5XF8kw3xO7cUl+dzUIW3B21emwcX6ocmOCX0zhCNz TSy383WztJq7+B74BWH3lnBJ1NhafebR1Zu8QCH4= Received: from mail-ua1-x936.google.com (mail-ua1-x936.google.com [IPv6:2607:f8b0:4864:20::936]) by sourceware.org (Postfix) with ESMTPS id 9E76B3951C01 for ; Thu, 13 Jan 2022 14:19:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9E76B3951C01 Received: by mail-ua1-x936.google.com with SMTP id l15so11249607uai.11 for ; Thu, 13 Jan 2022 06:19:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=LJeaNxfsZB8KGTmysyMM3Nq2dvWxfDyeTzRVV/Tx5to=; b=X4QKGwDtg8tOIfWsp089HPmcuMFrZICDjpt0SoA5AgtCg5SEjM/mnEIc+7NJqV1+jO sbMfnkkEsF7nGjsKPRKsXRWIWtkngICJvunExAZ6bsl5VZiL1ajnvaG8KEsouGeynzS6 YsmZsyyIXEadH0P/JXC7W0xbGrzDuyALhwQ9RIp1FfOEt52vjFL2+z909cvjoImXHzh0 tpBw2jW+VoGy1VexM2FnaQFYhyvgzVF6cfppwyc9IZ5cjB/xb9dso7NzJEfOR98M+vde mTaYCF/hNkTK8liijJZXN84J4PS+RkjuzZUNd/N90rfAkKvZ2W0rQofdPlVVSegyGDeu RO5w== X-Gm-Message-State: AOAM530Nuh87hZMD439kPczejImZtleks47+u4eXBCvVE4UmETUMfyVF TfBCQRrr3zL3GTX3fBQ7batRRUoHdC/woA== X-Google-Smtp-Source: ABdhPJw6H9vdHO0tF51QilTvKny/087Fe3gBKasaO2nisE0ujerUkf0WHxcvIRxFleNts85kFLLARw== X-Received: by 2002:a67:ff15:: with SMTP id v21mr2256040vsp.40.1642083545129; Thu, 13 Jan 2022 06:19:05 -0800 (PST) Received: from ?IPv6:2804:7f0:4841:563b:7573:30b6:b1a4:360f? ([2804:7f0:4841:563b:7573:30b6:b1a4:360f]) by smtp.gmail.com with ESMTPSA id ay30sm322840vkb.38.2022.01.13.06.19.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 13 Jan 2022 06:19:04 -0800 (PST) Subject: Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8 To: Andrew Burgess References: <20220104172254.3665546-1-luis.machado@linaro.org> <20220111212219.4018309-1-luis.machado@linaro.org> <20220112111434.GL622389@redhat.com> Message-ID: <0c22255a-15ec-9e36-918a-cb6a26adb488@linaro.org> Date: Thu, 13 Jan 2022 11:19:01 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20220112111434.GL622389@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 1/12/22 8:14 AM, Andrew Burgess wrote: > * Luis Machado via Gdb-patches [2022-01-11 18:22:19 -0300]: > >> When running gdb.cp/non-trivial-retval.exp, the following shows up for >> AArch64-Linux: >> >> Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35 >> 35 A a; >> (gdb) finish >> Run till exit from #0 f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35 >> main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163 >> 163 B b = f2 (i1, i2); >> Value returned is $6 = {a = -11952} >> (gdb) >> >> The return value should be {a = 123} instead. This happens because the AArch64 >> backend doesn't extract the return value from the correct location. GDB should >> fetch a pointer to the memory location from X8. >> >> With the patch, gdb.cp/non-trivial-retval.exp has full passes on >> AArch64-Linux Ubuntu 20.04/18.04. >> >> The problem only shows up with the "finish" command. The "call" command >> works correctly and displays the correct return value. >> >> This is also related to PR gdb/28681 >> (https://sourceware.org/bugzilla/show_bug.cgi?id=28681). >> --- >> gdb/aarch64-tdep.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index 63d626f90ac..0efb3834584 100644 >> --- a/gdb/aarch64-tdep.c >> +++ b/gdb/aarch64-tdep.c >> @@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type) >> return 0; >> } >> >> - if (TYPE_LENGTH (type) > 16) >> + if (TYPE_LENGTH (type) > 16 >> + || !language_pass_by_reference (type).trivially_copyable) >> { >> /* PCS B.6 Aggregates larger than 16 bytes are passed by >> invisible reference. */ >> @@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value, >> { >> if (aarch64_return_in_memory (gdbarch, valtype)) >> { >> + /* From the AAPCS64's Result Return section: >> + >> + "Otherwise, the caller shall reserve a block of memory of >> + sufficient size and alignment to hold the result. The address >> + of the memory block shall be passed as an additional argument to >> + the function in x8. */ >> + >> aarch64_debug_printf ("return value in memory"); >> - return RETURN_VALUE_STRUCT_CONVENTION; >> + >> + if (readbuf) >> + { >> + CORE_ADDR addr; >> + >> + regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr); >> + read_memory (addr, readbuf, TYPE_LENGTH (valtype)); >> + } >> + >> + return RETURN_VALUE_ABI_RETURNS_ADDRESS; > > So now, anything that should be returned in memory is of type > RETURN_VALUE_ABI_RETURNS_ADDRESS. This is interesting because it > should have implications outside of gdb.cp/non-trivial-retval.exp. Right. That's what I thought as well. > > In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for > most 64-bit targets, would normally be passed in registers, but which, > for aarch64 is required to be passed in memory. For AArch64, the Generic C++ ABI states returning non-trivially-copyable objects is done via a reference in r8. Doesn't x86_64 have a similar rule? > > After this change I would expect larger structs (> 16 bytes) to now > also work correctly in aarch64. Did you see any additional tests > starting to pass after this commit? For example, given this test > program: The curious thing is that I did not notice big changes in the testsuite results, only the FAIL->PASS transition from this particular test. I theorized this must be because, although we exercise function calling quite a bit, we do not exercise "finish" as much. Otherwise we would've seen this problem, given RETURN_VALUE_STRUCT_CONVENTION produces a nullptr result value. > > struct large_t > { > int array[32]; > }; > > struct large_t > func () > { > int i; > struct large_t obj; > > for (i = 0; i < 32; ++i) > obj.array[i] = i; > > return obj; > } > > int > main () > { > struct large_t obj = func (); > return obj.array[0]; > } > > On x86-64 this is what I see: > > $ gdb -q large-struct > Reading symbols from large-struct... > (gdb) set print elements 10 > (gdb) break func > Breakpoint 1 at 0x401116: file large-struct.c, line 12. > (gdb) r > Starting program: /tmp/large-struct > > Breakpoint 1, func () at large-struct.c:12 > 12 for (i = 0; i < 32; ++i) > (gdb) finish > Run till exit from #0 func () at large-struct.c:12 > main () at large-struct.c:22 > 22 return obj.array[0]; > Value returned is $1 = { > array = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9...} > } > (gdb) > > I would expect on aarch64 that the finish didn't work correctly before > this patch, but now does. Is this what you see? Before the patch it doesn't return any value, as expected: (gdb) finish Run till exit from #0 func () at test.c:8 main () at test.c:22 22 return obj.array[0]; Value returned has type: struct large_t. Cannot determine contents (gdb) The patch fixes it and makes GDB produce the output you pasted for x86_64. > > If you did see other tests starting to pass then could you mention > them in the commit message please. If not, could you add a test like > the above to the testsuite. I'll check the coverage for the "finish" command. Maybe exercise "finish" alongside manual function calls. Let me investigate that to see if I can come up with a useful testcase. > > Otherwise, the change looks good to me. > > Thanks, > Andrew >