From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id A4F4A39518B9 for ; Thu, 13 Jan 2022 15:18:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A4F4A39518B9 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-75-37iEMn3qNcKH_ody9GWXug-1; Thu, 13 Jan 2022 10:18:05 -0500 X-MC-Unique: 37iEMn3qNcKH_ody9GWXug-1 Received: by mail-wr1-f70.google.com with SMTP id t13-20020adfa2cd000000b001d09443ee4fso451022wra.3 for ; Thu, 13 Jan 2022 07:18:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wCUDhbMdEpyIIMJMksPIkXmJKvlAMWWMeJ+yrVzds4E=; b=e+sz3934iwY0UmcDchpatDosjnd4bcps0dUlsWoGbajYpgdUNh8uJpz9agpM5IU8Pu pkL1Wd1NWpkvs8EZOSFJDUYCBjJ7mOr9XmOjVmcuhCimmupeDP9DoneN8VCMmZsrLLVc cpOZnXHV9s3SRys3NHFdn+37uUxH/vexQH5013JVOZl3kw3yAOXrxzX64/33U7h9NMnJ oYqI+ObPKpYp+BeESxgB329Bg6D6PtV8uyTeHclrY9CMK4aPHCVB0Y3byuaPd6H0/UZu Ig3OQPmUxK98cPuwlRh8xudF5PE+b122+8xtOnhf5RWz7wh3YPURmzgEkMMm7r5XhXqg uGZA== X-Gm-Message-State: AOAM532HNw2mxkHyl6TTiRqwgPPpJmkVGKsqPvkHHV9vjqlFTlDHKgwc BL6u6rJm3mjBXR/jt/lukfmEPE03/dOR4+l3TO5PJZ/W4q71fNIBkW1RexzgCiAy9lYNACo1IVC tPqeckqV6+Xn8Ekv+xClm8Q== X-Received: by 2002:a1c:f205:: with SMTP id s5mr4442167wmc.33.1642087083629; Thu, 13 Jan 2022 07:18:03 -0800 (PST) X-Google-Smtp-Source: ABdhPJzbGT7ADwYzrJlT72ume96Xt+NAKN9LGINUStlqc3/xH1lKNpv9qq/QEOdwCj9yT5LbpZ8biA== X-Received: by 2002:a1c:f205:: with SMTP id s5mr4442143wmc.33.1642087083322; Thu, 13 Jan 2022 07:18:03 -0800 (PST) Received: from localhost (host86-188-49-82.range86-188.btcentralplus.com. [86.188.49.82]) by smtp.gmail.com with ESMTPSA id d16sm3187526wrq.27.2022.01.13.07.18.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Jan 2022 07:18:02 -0800 (PST) Date: Thu, 13 Jan 2022 15:18:01 +0000 From: Andrew Burgess To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8 Message-ID: <20220113151801.GW622389@redhat.com> 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> MIME-Version: 1.0 In-Reply-To: <0c22255a-15ec-9e36-918a-cb6a26adb488@linaro.org> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 15:12:52 up 12 days, 6 min, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Thu, 13 Jan 2022 15:18:11 -0000 * 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. 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 :) Thanks, Andrew