From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121236 invoked by alias); 2 Sep 2019 18:20:52 -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 121226 invoked by uid 89); 2 Sep 2019 18:20:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.9 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.1 spammy=Starting X-HELO: mail-wr1-f49.google.com Received: from mail-wr1-f49.google.com (HELO mail-wr1-f49.google.com) (209.85.221.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Sep 2019 18:20:50 +0000 Received: by mail-wr1-f49.google.com with SMTP id g7so14914245wrx.2 for ; Mon, 02 Sep 2019 11:20:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=uOdbFtW7A7UIHUZ1XI+nScoHVG42MoVAheKlKmf0guM=; b=KR9fEvIod68WQKbRUoEnpCAThV84N37mf0IEe1pXJy4k7x61/LhsYoj5iiSkCujWso zzgQDo1H8zSkpx2dsUDTAiwlUxahALK68Eb7dzrUGpOu3nNeDEs0iUyhIsMN5CYmvKV0 RlCrNFqpkhTKaz01a3DPP768MYPWihOH9qw3tPKh64RKZvjLaGtUInAa+OD8uxAs4s3A HHY7G51LwLRZryPgub0/W8ZduX1ar6kYTQVp+xwX8juCna0yVWO4WdFy5F6FtS3lJJcs U8ApUV/zxRViC8D3iNXFwkGC7wR5uUo2t0MABk1jcIiRlMInh4naPy4sfZuYjgv4GiJS rrSA== Return-Path: Received: from localhost (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by smtp.gmail.com with ESMTPSA id x5sm9788870wrg.69.2019.09.02.11.20.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 02 Sep 2019 11:20:47 -0700 (PDT) Date: Mon, 02 Sep 2019 18:20:00 -0000 From: Andrew Burgess To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH][gdb/testsuite] Allow some tests in gdb.base/store.exp to be unsupported Message-ID: <20190902182045.GC6076@embecosm.com> References: <20190829174710.GA5656@delia> <20190829180736.GY6076@embecosm.com> <90e042e0-824c-7e70-a0a4-5ce91e79c7dc@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <90e042e0-824c-7e70-a0a4-5ce91e79c7dc@suse.de> X-Fortune: Different all twisty a of in maze are you, passages little. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00011.txt.bz2 * Tom de Vries [2019-09-02 13:00:21 +0200]: > [ was: Re: [PATCH][gdb/testsuite] Rewrite gdb.base/store.exp into .s test ] > > On 29-08-19 20:07, Andrew Burgess wrote: > > * Tom de Vries [2019-08-29 19:47:12 +0200]: > > > >> Hi, > >> > >> The test-case gdb.base/store.exp fails with gcc 7.4.0: > >> ... > >> nr of unexpected failures 27 > >> ... > >> and with gcc 4.8.5: > >> ... > >> nr of unexpected failures 9 > >> ... > >> > >> The test-case relies on "the compiler taking heed of requests for values to be > >> stored in registers", which appearantly isn't the case anymore for > >> modern gcc. > > > > Could you expand on this a little more. I took a quick look and it > > appears more that variables just have missing location information. > > You're right, I jumped to a conclusion here, sorry for not being more > careful. > > > Sure the test marks the variables with the 'register' keyword, but > > surely GDB should still pass the test even if the variable is placed > > on the stack? > > > > Agreed (and indeed, not obeing the 'register' hint, emulated by "#define > register" makes the test pass). > > >> > >> Fix this by changing this into an assembly file test-case, and generating the > >> assembly file using gcc 4.2.1. > >> > >> Tested on x86_64-linux. > >> > >> OK for trunk? > > > > No. What about architectures other than x86-64? > > > > I took a quick look, and maybe I missed something, but I don't think > > that there are any architecture specific assembler tests in gdb.base/ > > and I don't think we should be adding any. > > > > Maybe we should add a version of this test into gdb.arch along with > > the assembler file for x86-64. > > > > On a slightly different note, I've run into this test before, and I'm > > pretty sure that the test is broken, it's been a while since I dug > > into this but consider these snippets: > > > > ... > > > > float > > add_float (register float u, register float v) > > { > > return u + v; > > } > > > > ... > > > > wack_float (register float u, register float v) > > { > > register float l = u, r = v; > > l = add_float (l, r); > > return l + r; > > } > > > > ... > > > > Part of the test involves breaking on 'add_float' then going 'up' to > > 'wack_float' and printing 'l'. GDB expects an answer. > > > > My problem with this is that on many architectures, even at > > optimisation level 0 'l' is dead, or at least non-recoverable at the > > point of the call to add_float due to being placed in a caller saved > > argument register. > > > > I've investigated the FAILs related to the wack_float function, and the > test-case expects to access and modify l, but it can't because there's > no DW_AT_location for l, which AFAIU is valid behaviour of gcc for a > register variable at -O0. I don't understand why this should be the case. DWARF is perfectly able to describe the location of something in a register, so why would it be valid for GCC to skip emitting location information for something just because it's in a register? > > So, ISTM the FAILs need to be fixed by marking the failing tests as > unsupported, in case l shows up as . I guess unsupported, but maybe KFAIL with an associated GCC bug would be better? Consider this GDB session to explain my thinking: (gdb) break wack_float Breakpoint 1 at 0x4005c8: file /path/to/gdb/src/gdb/testsuite/gdb.base/store.c, line 109. (gdb) r Starting program: /path/to/gdb/build/gdb/testsuite/outputs/gdb.base/store/store Breakpoint 1, wack_float (u=-1, v=-2) at /path/to/gdb/src/gdb/testsuite/gdb.base/store.c:109 109 register float l = u, r = v; (gdb) n 110 l = add_float (l, r); (gdb) p u $1 = -1 (gdb) p l $2 = (gdb) We know where 'u' is, as I see it there's no reason why GCC couldn't emit location information for 'l' that is identical to that for 'u'. [ OK, if we're going to be supper picky then GCC could declare 'u' dead after line 109 and then have only 'l' exist after that, but at -O0 I'd probably hope that both 'u' and 'l' would remain alive for the entire life of the function. ] Thanks, Andrew > > > Anyway, I agree with you that this test is in need of some clean up, > > I'm just not convinced on this approach yet. > > > > Better like this? > > Thanks, > - Tom > [gdb/testsuite] Allow some tests in gdb.base/store.exp to be unsupported > > The test-case gdb.base/store.exp fails with gcc 7.4.0: > ... > nr of unexpected failures 27 > ... > > The first FAIL: > ... > 110 l = add_float (l, r); > (gdb) PASS: gdb.base/store.exp: continue to wack_float > print l > $21 = > FAIL: gdb.base/store.exp: var float l; print old l, expecting -1 > ... > relates to this bit in the test-case (compiled at -O0): > ... > 106 float > 107 wack_float (register float u, register float v) > 108 { > 109 register float l = u, r = v; > 110 l = add_float (l, r); > 111 return l + r; > 112 } > ... > and it expects to be able to read and modify variable l before executing line > 110, but it already fails to read the value, because l has no DW_AT_location > attribute in the debug info. > > Variable l is declared with the register keyword, and GCC implements the > register keyword at -O0 like so: > ... > the compiler allocates distinct stack memory for all variables that do not > have the register storage-class specifier; if register is specified, the > variable may have a shorter lifespan than the code would indicate and may > never be placed in memory. > ... > > The fact that l has no DW_AT_location attribute, matches with the documented > "variable may have a shorter lifespan that code would indicate", (though it > is the most extreme case of it) so the gcc behaviour is valid. We can of > course improve gcc to generate better debuginfo (filed gcc PR91611), but > this not a wrong-debug problem. > > [ The test-case passes with gcc 4.2.1, but for the failing test discussed > above, it passes simply because it doesn't store l in a register. ] > > With the debug info missing for l, reading and setting l is unsupported, so > fix the FAIL by marking the test UNSUPPORTED instead. > > Tested on x86_64-linux. > > gdb/testsuite/ChangeLog: > > 2019-08-29 Tom de Vries > > * gdb.base/store.exp: Allow register variables to be optimized out at > -O0. > > --- > gdb/testsuite/gdb.base/store.exp | 65 +++++++++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 20 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp > index c5a7584101..9c19ce15a7 100644 > --- a/gdb/testsuite/gdb.base/store.exp > +++ b/gdb/testsuite/gdb.base/store.exp > @@ -55,18 +55,29 @@ proc check_set { t l r new add } { > } > } > > - gdb_test "print l" " = ${l}" \ > - "${prefix}; print old l, expecting ${l}" > - gdb_test "print r" " = ${r}" \ > - "${prefix}; print old r, expecting ${r}" > - gdb_test_no_output "set variable l = 4" \ > - "${prefix}; setting l to 4" > - gdb_test "print l" " = ${new}" \ > - "${prefix}; print new l, expecting ${new}" > - gdb_test "next" "return l \\+ r;" \ > - "${prefix}; next over add call" > - gdb_test "print l" " = ${add}" \ > - "${prefix}; print incremented l, expecting ${add}" > + set supported 1 > + set test "${prefix}; print old l, expecting ${l}" > + gdb_test_multiple "print l" "$test" { > + -re " = \r\n$gdb_prompt $" { > + unsupported $test > + set supported 0 > + } > + -re " = ${l}\r\n$gdb_prompt $" { > + pass $test > + } > + } > + if { $supported } { > + gdb_test "print r" " = ${r}" \ > + "${prefix}; print old r, expecting ${r}" > + gdb_test_no_output "set variable l = 4" \ > + "${prefix}; setting l to 4" > + gdb_test "print l" " = ${new}" \ > + "${prefix}; print new l, expecting ${new}" > + gdb_test "next" "return l \\+ r;" \ > + "${prefix}; next over add call" > + gdb_test "print l" " = ${add}" \ > + "${prefix}; print incremented l, expecting ${add}" > + } > } > > check_set "charest" "-1 .*" "-2 .*" "4 ..004." "2 ..002." > @@ -81,20 +92,34 @@ check_set "doublest" "-1" "-2" "4" "2" > # > > proc up_set { t l r new } { > + global gdb_prompt > + > set prefix "upvar ${t} l" > gdb_test "tbreak add_${t}" > gdb_test "continue" "return u . v;" \ > "continue to add_${t}" > gdb_test "up" "l = add_${t} .l, r.;" \ > "${prefix}; up" > - gdb_test "print l" " = ${l}" \ > - "${prefix}; print old l, expecting ${l}" > - gdb_test "print r" " = ${r}" \ > - "${prefix}; print old r, expecting ${r}" > - gdb_test_no_output "set variable l = 4" \ > - "${prefix}; set l to 4" > - gdb_test "print l" " = ${new}" \ > - "${prefix}; print new l, expecting ${new}" > + > + set supported 1 > + set test "${prefix}; print old l, expecting ${l}" > + gdb_test_multiple "print l" "$test" { > + -re " = \r\n$gdb_prompt $" { > + unsupported $test > + set supported 0 > + } > + -re " = ${l}\r\n$gdb_prompt $" { > + pass $test > + } > + } > + if { $supported } { > + gdb_test "print r" " = ${r}" \ > + "${prefix}; print old r, expecting ${r}" > + gdb_test_no_output "set variable l = 4" \ > + "${prefix}; set l to 4" > + gdb_test "print l" " = ${new}" \ > + "${prefix}; print new l, expecting ${new}" > + } > } > > up_set "charest" "-1 .*" "-2 .*" "4 ..004."