Message ID | 1541632206-143680-1-git-send-email-brobecker@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 92924 invoked by alias); 7 Nov 2018 23:10:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 92909 invoked by uid 89); 7 Nov 2018 23:10:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:2796, 425, pck.adb, UD:pck.adb X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Nov 2018 23:10:18 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3497456103; Wed, 7 Nov 2018 18:10:17 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id g854tklz8aQz; Wed, 7 Nov 2018 18:10:17 -0500 (EST) Received: from tron.gnat.com (tron.gnat.com [205.232.38.10]) by rock.gnat.com (Postfix) with ESMTP id 2512956102; Wed, 7 Nov 2018 18:10:17 -0500 (EST) Received: by tron.gnat.com (Postfix, from userid 4233) id 095243517; Wed, 7 Nov 2018 18:10:17 -0500 (EST) From: Joel Brobecker <brobecker@adacore.com> To: gdb-patches@sourceware.org Cc: Alan Hayward <alan.hayward@arm.com> Subject: [RFA] (AArch64) wrong value returned by "finish" for HFA Date: Wed, 7 Nov 2018 18:10:06 -0500 Message-Id: <1541632206-143680-1-git-send-email-brobecker@adacore.com> |
Commit Message
Joel Brobecker
Nov. 7, 2018, 11:10 p.m. UTC
Hello, Consider the gdb.ada/array_return.exp testcase, and in particular, consider the following code... type Small_Float_Vector is array (1 .. 2) of Float; function Create_Small_Float_Vector return Small_Float_Vector is begin return (others => 4.25); end Create_Small_Float_Vector; ... which declares a type which is an array with 2 floats in it (floats are 4 bytes on AArch64), trying to get GDB to print the return value from that function does not work: (gdb) fin Run till exit from #0 pck.create_small_float_vector () at /[...]/pck.adb:15 0x000000000000062c in p () at /[...]/p.adb:11 11 Vector := Create_Small_Float_Vector; Value returned is $1 = (4.25, 0.0) ^^^ ||| We expected the value shown to be: (gdb) fin Run till exit from #0 pck.create_small_float_vector () at /[...]/pck.adb:15 0x000000000000062c in p () at /[...]/p.adb:11 11 Vector := Create_Small_Float_Vector; Value returned is $1 = (4.25, 4.25) Because the return type is an HFA, it is returned via the first two SIMD registers. However, what happens is that the current implementation fails to realize that this is an HFA, and therefore fetches the return value from the wrong location. And the reason why it fails to realize this is because it thinks that our array has 8 elements (HFAs have a maximum of 4). Looking at aapcs_is_vfp_call_or_return_candidate_1, where this is determined, we can easily see why (looks like a thinko): | case TYPE_CODE_ARRAY: | [...] | struct type *target_type = TYPE_TARGET_TYPE (type); | int count = aapcs_is_vfp_call_or_return_candidate_1 | (target_type, fundamental_type); | | if (count == -1) | return count; | !! -> | count *= TYPE_LENGTH (type); | return count; Here, we first determine the count for one element of our array, and so we should then be multiplying that count by the number of elements in our array (2 in our case). But instead, we multiply it by the total size (8). As a result, we do not classify the return type as an HFA, and thus pick the wrong location for fetching the return value. gdb/ChangeLog: * aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): return the correct count for potential HFAs. Tested on aarch64-linux, fixes: array_return.exp: value printed by finish of Create_Small_Float_Vector OK to commit? Thank you,
Comments
> On 7 Nov 2018, at 23:10, Joel Brobecker <brobecker@adacore.com> wrote: > > Hello, > > Consider the gdb.ada/array_return.exp testcase, and in particular, > consider the following code... > > type Small_Float_Vector is array (1 .. 2) of Float; > > function Create_Small_Float_Vector return Small_Float_Vector is > begin > return (others => 4.25); > end Create_Small_Float_Vector; > > ... which declares a type which is an array with 2 floats in it > (floats are 4 bytes on AArch64), trying to get GDB to print > the return value from that function does not work: > > (gdb) fin > Run till exit from #0 pck.create_small_float_vector () at /[...]/pck.adb:15 > 0x000000000000062c in p () at /[...]/p.adb:11 > 11 Vector := Create_Small_Float_Vector; > Value returned is $1 = (4.25, 0.0) > ^^^ > ||| > > We expected the value shown to be: > > (gdb) fin > Run till exit from #0 pck.create_small_float_vector () at /[...]/pck.adb:15 > 0x000000000000062c in p () at /[...]/p.adb:11 > 11 Vector := Create_Small_Float_Vector; > Value returned is $1 = (4.25, 4.25) > > Because the return type is an HFA, it is returned via the first two > SIMD registers. However, what happens is that the current implementation > fails to realize that this is an HFA, and therefore fetches the return > value from the wrong location. And the reason why it fails to realize > this is because it thinks that our array has 8 elements (HFAs have > a maximum of 4). Looking at aapcs_is_vfp_call_or_return_candidate_1, > where this is determined, we can easily see why (looks like a thinko): > > | case TYPE_CODE_ARRAY: > | [...] > | struct type *target_type = TYPE_TARGET_TYPE (type); > | int count = aapcs_is_vfp_call_or_return_candidate_1 > | (target_type, fundamental_type); > | > | if (count == -1) > | return count; > | > !! -> | count *= TYPE_LENGTH (type); > | return count; > > Here, we first determine the count for one element of our array, > and so we should then be multiplying that count by the number > of elements in our array (2 in our case). But instead, we multiply it > by the total size (8). As a result, we do not classify the return > type as an HFA, and thus pick the wrong location for fetching > the return value. > Apologies, this was my goof. Fix looks good to me. > gdb/ChangeLog: > > * aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): > return the correct count for potential HFAs. > > Tested on aarch64-linux, fixes: > > array_return.exp: value printed by finish of Create_Small_Float_Vector I had to install gnat to test this (which is probably why I never spotted the fail before). Ideally, this case should have been tested as part of gdb.base/infcall-nested-structs.exp. As a side note, I’ve also got a work in progress fix for this area of code to fix gdb.dwarf2/dw2-cp-infcall-ref-static.exp which uses a recursive type written in dwarf. > > OK to commit? > > Thank you, > -- > Joel > > --- > gdb/aarch64-tdep.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 0211930..51f1632 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -1212,7 +1212,7 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type, > if (count == -1) > return count; > > - count *= TYPE_LENGTH (type); > + count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type)); > return count; > } > } > -- > 2.1.4 >
> Apologies, this was my goof. Fix looks good to me. Thank you. Considering that I really like what you did to the code in that respect, please do not apologize! ;-) > > gdb/ChangeLog: > > > > * aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): > > return the correct count for potential HFAs. > > > > Tested on aarch64-linux, fixes: > > > > array_return.exp: value printed by finish of Create_Small_Float_Vector Pushed to master. > I had to install gnat to test this (which is probably why I never > spotted the fail before). I don't think it was passing before either. AdaCore had a local patch to handle this that Tristan tried to submit. But, at the time, all we could do to validate it was run it through AdaCore's testsuite, and the maintainer at the time asked that it be run through the official testsuite, which would have required a bit of up-front work that we never got around to doing. This time around, one of my coworkers had a compiler handy for me, so it was less effort :). > Ideally, this case should have been tested > as part of gdb.base/infcall-nested-structs.exp. I thought about that and got started on writing a C testcase for it. Except that C doesn't allow return values that are arrays. But thinking about it more, with AArch64, we have the advantage that the calling convention for return values are the same as for arguments, so we could exercise the same scenario using an inferior function call with one argument being an array of floats!
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index 0211930..51f1632 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -1212,7 +1212,7 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type, if (count == -1) return count; - count *= TYPE_LENGTH (type); + count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type)); return count; } }