From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30949 invoked by alias); 24 Apr 2018 07:18:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30940 invoked by uid 89); 24 Apr 2018 07:18:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: aserp2120.oracle.com Received: from aserp2120.oracle.com (HELO aserp2120.oracle.com) (141.146.126.78) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 24 Apr 2018 07:18:38 +0000 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w3O7FnLk158524; Tue, 24 Apr 2018 07:18:32 GMT Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by aserp2120.oracle.com with ESMTP id 2hfw9a8gjc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Apr 2018 07:18:32 +0000 Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w3O7IV3n011452 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Apr 2018 07:18:32 GMT Received: from abhmp0013.oracle.com (abhmp0013.oracle.com [141.146.116.19]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w3O7IUWd011889; Tue, 24 Apr 2018 07:18:31 GMT Received: from [10.159.247.75] (/10.159.247.75) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 24 Apr 2018 00:18:30 -0700 Subject: Re: [RFA/commit] (SPARC/LEON) fix incorrect array return value printed by "finish" To: Joel Brobecker , gdb-patches@sourceware.org References: <1524505529-79109-1-git-send-email-brobecker@adacore.com> From: vladimir.mezentsev@oracle.com Message-ID: Date: Tue, 24 Apr 2018 07:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1524505529-79109-1-git-send-email-brobecker@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8872 signatures=668698 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=2 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1804240074 X-SW-Source: 2018-04/txt/msg00456.txt.bz2 On 04/23/2018 10:45 AM, Joel Brobecker wrote: > Consider the code in the gdb.ada/array_return.exp testcase, which > defines a function returning an array of 2 integers: > > type Data_Small is array (1 .. 2) of Integer; > function Create_Small return Data_Small; > > When doing a "finish" from inside function Create_Small, we expect > GDB to tell us that the return value was "(1, 1)". However, it currently > prints the wrong value: > > (gdb) finish > Run till exit from #0 pck.create_small () at /[...]/pck.adb:5 > p () at /[...]/p.adb:10 > 10 Large := Create_Large; > Value returned is $1 = (0, 0)  I never used  ada and it looks like a bug in ada compiler not in gdb. Probably ada generates incorrect code for function which returns a small array. The similar c  test works: % cat r.c #include typedef int __attribute__ ((vector_size (2 * sizeof(int)))) I2; I2 func() {     I2 a;     a[0] = 11;     a[1] = 12;     return a; } int main() {     I2 x = func();     printf("%d %d\n", x[0], x[1]);     return 0; } % gcc -g r.c % ./a.out 11 12 % /export/home/vmezents/git/obj_binutils-gdb/gdb/gdb ./a.out GNU gdb (GDB) 8.0.50.20170512-git Copyright (C) 2017 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law.  Type "show copying" and "show warranty" for details. This GDB was configured as "sparc64-unknown-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: . Find the GDB manual and other documentation resources online at: . For help, type "help". Type "apropos word" to search for commands related to "word"... Resuming the execution of threads of all processes is on. Reading symbols from ./a.out...done. (gdb) b main Breakpoint 1 at 0x1006a8: file r.c, line 13. (gdb) run Starting program: /export/home/vmezents/BUGs/25502809/a.out Breakpoint 1, main () at r.c:13 13        I2 x = func(); (gdb) print func() $1 = {11, 12} See also  my comment below. > This is a regression which I traced back to the following commit... > > | commit 1933fd8ee01ad2e74a9c6341bc40f54962a8f889 > | Date: Fri May 19 03:06:19 2017 -0700 > | Subject: gdb: fix TYPE_CODE_ARRAY handling in sparc targets > > ... which, despite what the subject says, is not really about > TYPE_CODE_ARRAY handling, which is a bit of an implementation detail, > but about the GNU vectors extension. > > The author of the patch equated TYPE_CODE_ARRAY with vectors, which > is not correct. Vectors are TYPE_CODE_ARRAY types with the TYPE_VECTOR > flag set. So at the very minimum, the patch should have been checking > for both TYPE_CODE_ARRAY and TYPE_VECTOR. > > But, that's not the only thing that did not seem right to me. When > looking at the ABI, and at the summary of the implementation in GCC > of the calling conventions for that architecture: > > size argument return value > > small integer <4 int. reg. int. reg. > word 4 int. reg. int. reg. > double word 8 int. reg. int. reg. > > _Complex small integer <8 int. reg. int. reg. > _Complex word 8 int. reg. int. reg. > _Complex double word 16 memory int. reg. > > vector integer <=8 int. reg. FP reg. > vector integer >8 memory memory > > float 4 int. reg. FP reg. > double 8 int. reg. FP reg. > long double 16 memory memory > > _Complex float 8 memory FP reg. > _Complex double 16 memory FP reg. > _Complex long double 32 memory FP reg. > > vector float any memory memory > > aggregate any memory memory  I think the 4 from last 5 are incorrect (See for example https://www.gnu.org/software/gcc/gcc-3.4/sparc-abi.html) It should be: _Complex float 8 reg FP reg. _Complex double 16 reg FP reg. vector float <=16 reg reg aggregate <=16 reg reg  It will be good to add comments with the correct table into  gdb/sparc-tdep.c. -Vladimir > > The nice thing about the patch above is that it nicely factorized > the code that determines how arguments are passed/returns. The bad > news is that the implementation, particularly for the handling of > arrays and vectors, doesn't seem to match the summary above. Hence, > the regression we observed. > > So what I did was review and re-implement some of the predicate functions > according to the summary above. Because dejagnu crashes all our Solaris > machines real bad, I can't run the dejagnu testsuite there. So what I did > was test the patch with AdaCore's testsuite against leon3-elf, no > regression. I verified that this fixes the regression above while > at the same time still passing gdb.base/gnu_vector.exp (I transposed > that testcase to our testsuite), which is the testcase that was cited > in the commit above as seeing some FAIL->PASS improvements. > > This patch also removes one assertion... > > gdb_assert (sparc_integral_or_pointer_p (type) > || (TYPE_CODE (type) == TYPE_CODE_ARRAY && len <= 8)); > > ... because that assertion is really the "negative" of the other conditions > written in the same "if, else if, else [assert]" block in this function. > To me, this assertion forces us to maintain two versions of the same code, > and is an unnecessary burden. In particular, the above is not the > correct condition, and the ABI summary table above shows that we need > a more complex condition to describe the situations where we expect > arguments to be passed by register. > > gdb/ChangeLog: > > * sparc-tdep.c (sparc_structure_return_p): Re-implement to > match the ABI as summarized in GCC's gcc/config/sparc/sparc.c. > (sparc_arg_by_memory_p): Renamed from sparc_arg_on_registers_p. > Re-implement to match the ABI as summarized in GCC's > gcc/config/sparc/sparc.c. All callers updated. > (sparc32_store_arguments): Remove assertion. > > OK to commit in master? > > Thanks!