From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id cIpwOo1E4GEqQwAAWB0awg (envelope-from ) for ; Thu, 13 Jan 2022 10:26:05 -0500 Received: by simark.ca (Postfix, from userid 112) id ED8521F34E; Thu, 13 Jan 2022 10:26:05 -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 119DA1ECEB for ; Thu, 13 Jan 2022 10:26:05 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AA5563951CB6 for ; Thu, 13 Jan 2022 15:26:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AA5563951CB6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1642087564; bh=BUcbTQjZqukAsZG9M/y7dYkeJgvqI6x998rNAwxEn5A=; 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=QitGWoNc+pROnR4jvQlBzBHlX5hVZOQGMtb/2rlWyJqFWnyKGExAD6rYw3gJ3sgmn b6hrs/nd2TG0vvkaXaZZ81wH54GbHRC4SyoIuJteDL1DD6Kdv9NuM9oAfJfjb5aPHS /I6iesC8NDC0c7DcqpPg7mT8Yb4nW70sfzIMm9b8= Received: from mail-vk1-xa35.google.com (mail-vk1-xa35.google.com [IPv6:2607:f8b0:4864:20::a35]) by sourceware.org (Postfix) with ESMTPS id 5E89B3951C31 for ; Thu, 13 Jan 2022 15:22:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5E89B3951C31 Received: by mail-vk1-xa35.google.com with SMTP id v192so1035040vkv.4 for ; Thu, 13 Jan 2022 07:22:35 -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=BUcbTQjZqukAsZG9M/y7dYkeJgvqI6x998rNAwxEn5A=; b=nri9uk+dTvV/nxCLZbjHya6uVWFjC8gHRzzMD4eJmI0cw9pTjwistSpm9FlGztS0Se kLIYbJs36Jw3Sy5tNdq4XHkkt+UpEONgfly02BWELB9F7Lxe+cNCMHiyUZei+e8P7JFU X7EXO5ZWQiNO+we5rfBxu1y1CdL+UEUFfZWUtJHaXCKGpBIT0fN/GGHI0Jzd50lKfi8K /FL52X9FU5HqfZT2TLDhgCBkCiJBt1RWMSmRu4xWryn7B1dsFn5DxfHrTa9bbbx69kys SgncAbuH5MLDyAJ5xWs5nmKcqwkTY5GzlQZRmowSMBtIJYzOvjWFP9gjNbxu1z5utZcN qzxw== X-Gm-Message-State: AOAM532y3kchkSHKBvJiuh6/iDjthYErdE82qNlSWKmzpkxXWDLal6XV 1TbIQsKGIRnTvX0ZQz5GfjhnvkZ8aIu9ZA== X-Google-Smtp-Source: ABdhPJyfDkIWLspCb6u+srPQUE8Cw/eWajb7AJ32QAEoZDBHJuvjJLPNG+hQwvhy7bsXlMTIX5GPZQ== X-Received: by 2002:a05:6122:2214:: with SMTP id bb20mr2445282vkb.9.1642087354767; Thu, 13 Jan 2022 07:22:34 -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 r9sm1562146vke.20.2022.01.13.07.22.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 13 Jan 2022 07:22:34 -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> <0c22255a-15ec-9e36-918a-cb6a26adb488@linaro.org> <20220113151801.GW622389@redhat.com> Message-ID: Date: Thu, 13 Jan 2022 12:22:31 -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: <20220113151801.GW622389@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/13/22 12:18 PM, Andrew Burgess wrote: > * Luis Machado via Gdb-patches [2022-01-13 11:19:01 -0300]: > >> 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? > > Sorry, I realised what I wrote wasn't what I meant. What I should > have said was: > > In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for > most 64-bit targets, would normally be passed in registers **if the > struct was trivially copyable**, but which, for aarch64 (and x86-64) > is required to be passed in memory. > Got it. Thanks for confirming this. > My point, which I think you've confirmed, is that the change you made, > correctly changes the behaviour for more than just small non-trivially > copyable structs; you also changed the behaviour for large structs. > This was clearly the right thing to do. > > All I'm suggesting is that you should add a test case to cover the > returning a large, trivially copyable struct as part of this patch, > clearly this isn't something we otherwise test. > >> >>> >>> 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. > > I didn't mean to create too much work for you - was just requesting > one extra test :) It is a tradition in GDB to have one or more bugs appear while fixing a single seemingly trivial bug. So no worries there. :-) I wasn't aware, at the time, the return convention was incorrect for all memory-returned values. It is worth having more coverage for sure.