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 4DAA03858D33 for ; Fri, 5 May 2023 16:21:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4DAA03858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683303715; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Os7ZhQSV692up+q4ejsDDxW0qjKO+gFJ3k69GmSyutM=; b=iLTBTxDQN3aCeT9+QxhStKCu4sm6QwvDfYIQjqSoWw/8abzRjKAJcsL9dd7lIfE41LQ2jx NNRSCBS3wTL9+cVVW9KxSI++Ow4dBwaB+OWbd2+bWaBi+fZ0sPGnsiBRuo1oNFmU4JgB1c a2bv/Q0dtv6xQc4GE1O/VEV3pGbeenw= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-427-0L1ySaN9O6WnKfcJ-Xp3jA-1; Fri, 05 May 2023 12:21:54 -0400 X-MC-Unique: 0L1ySaN9O6WnKfcJ-Xp3jA-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-3f1745d08b5so8748305e9.1 for ; Fri, 05 May 2023 09:21:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683303713; x=1685895713; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Os7ZhQSV692up+q4ejsDDxW0qjKO+gFJ3k69GmSyutM=; b=aCm6fqXkH56h/KgfWrk0KnXpir+qIWgcMvfPdYuIuz5MmkKuIHh3+WYs2OU+m5uG0l I1TsdEbL0GHT0351rj1kvh6opaUmw8DPyvsFZgzdFqueNgI7EtIT61TVeMswYo5XdTE7 rlsy65uU5xpVtvfn5Eo4TrWMmo2yzHYWNiO3nZ+h5F1DJ453KJErXIKQajHOZstGgUen t8eVzK/BKvkJOyN9ha77OQGFca8Qmo/74HqijTcxdMexscMtDT2vBlrS0YNvzL2GQovd 0eQMTDComwxmDCJ1zzD85YZh5Zc/2G2QKUuIWO3kzyYpY+Kkiq3FgVghvrjR6E6roK5m h//w== X-Gm-Message-State: AC+VfDzWVW+JH1nyxBMVdPXy/bbf7VWGAnqFk+Ouvt89ZlhXDewUu84S w5fBKLftMDuOSCZQnJIZ/+AkSV03ypmvCXcSnl/IOGptAm4l630dg2JdJU9+/iDOD4XsJ9S+1z4 jipMB+xElfLyB2a6wAX7FcwXp/OXpJQ== X-Received: by 2002:a05:600c:b4b:b0:3f2:73a:32fc with SMTP id k11-20020a05600c0b4b00b003f2073a32fcmr1550131wmr.32.1683303713325; Fri, 05 May 2023 09:21:53 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5viWIMezL0IaJLKBG/F8rxq5OJu4D9J9TtyERTBCu1LvBkVJ8mAlFt3F6GAydF9D+nDA7PTQ== X-Received: by 2002:a05:600c:b4b:b0:3f2:73a:32fc with SMTP id k11-20020a05600c0b4b00b003f2073a32fcmr1550115wmr.32.1683303713038; Fri, 05 May 2023 09:21:53 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id l14-20020a5d560e000000b003048084a57asm2833696wrv.79.2023.05.05.09.21.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 May 2023 09:21:52 -0700 (PDT) From: Andrew Burgess To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" Subject: RE: [PATCH 1/1] gdb: Avoid warning for the jump command inside an inline function. In-Reply-To: References: <20230124151932.2471769-1-felix.willgerodt@intel.com> <87o7ncawaj.fsf@redhat.com> Date: Fri, 05 May 2023 17:21:51 +0100 Message-ID: <87a5yi92ao.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Willgerodt, Felix" writes: >> -----Original Message----- >> From: Willgerodt, Felix >> Sent: Dienstag, 25. April 2023 16:40 >> To: Andrew Burgess ; gdb- >> patches@sourceware.org >> Subject: RE: [PATCH 1/1] gdb: Avoid warning for the jump command inside an >> inline function. >> >> > -----Original Message----- >> > From: Andrew Burgess >> > Sent: Dienstag, 25. April 2023 16:09 >> > To: Felix Willgerodt via Gdb-patches ; gdb- >> > patches@sourceware.org >> > Cc: Willgerodt, Felix ; Cristian Sandu >> > >> > Subject: Re: [PATCH 1/1] gdb: Avoid warning for the jump command inside >> > an inline function. >> > >> > Felix Willgerodt via Gdb-patches writes: >> > >> > > When stopped inside an inline function, trying to jump to a different line >> > > of the same function currently results in a warning about jumping to >> > another >> > > function. Fix this by taking inline functions into account. >> > > >> > > Before: >> > > Breakpoint 1, function_inline (x=510) at jump-inline.cpp:22 >> > > 22 a = a + x; /* inline-funct */ >> > > (gdb) j 21 >> > > Line 21 is not in `function_inline(int)'. Jump anyway? (y or n) >> > > >> > > After: >> > > Breakpoint 2, function_inline (x=510) at jump-inline.cpp:22 >> > > 22 a = a + x; /* inline-funct */ >> > > (gdb) j 21 >> > > Continuing at 0x400679. >> > > >> > > Breakpoint 1, function_inline (x=510) at jump-inline.cpp:21 >> > > 21 a += 1020 + a; /* increment-funct */ >> > > >> > > This was regression-tested on X86-64 Linux. >> > > >> > > Co-Authored-by: Cristian Sandu >> > > --- >> > > gdb/infcmd.c | 3 +- >> > > gdb/testsuite/gdb.base/jump-inline.c | 30 +++++++++++++++++ >> > > gdb/testsuite/gdb.base/jump-inline.exp | 45 >> > ++++++++++++++++++++++++++ >> > > 3 files changed, 77 insertions(+), 1 deletion(-) >> > > create mode 100644 gdb/testsuite/gdb.base/jump-inline.c >> > > create mode 100644 gdb/testsuite/gdb.base/jump-inline.exp >> > > >> > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c >> > > index fd88b8ca328..40414bc9260 100644 >> > > --- a/gdb/infcmd.c >> > > +++ b/gdb/infcmd.c >> > > @@ -1091,7 +1091,8 @@ jump_command (const char *arg, int from_tty) >> > > >> > > /* See if we are trying to jump to another function. */ >> > > fn = get_frame_function (get_current_frame ()); >> > > - sfn = find_pc_function (sal.pc); >> > > + sfn = find_pc_sect_containing_function (sal.pc, >> > > + find_pc_mapped_section (sal.pc)); >> > >> > I had a read through the discussion about whether find_pc_function >> > should return inline functions or not. I don't know the history of this >> > code, so I don't know if there's a reason why it does what it does, but >> > looking at how it's used, I know there are some places in GDB where >> > find_pc_function is used when it shouldn't be. >> > >> > For example in edit_command (cli-cmds.c) I'm pretty sure the use of >> > find_pc_function is incorrect -- GDB will print the name of the >> > containing function, but then open the editor on the inlined function. >> > >> > As was said elsewhere, the biggest problem here will be lack of >> > testing. What is really needed is an audit of each use of >> > find_pc_function and to ensure that each use is hit with an inline >> > function and a non-inline function. Until then I think it's hard to be >> > certain about whether find_pc_function can safely be changed or not. >> > But doing that is no small task. >> > >> > Given that, I'm inclined to think we should take the patch as >> > presented. If anyone ever does get around to sorting out this corner of >> > GDB then it's easy enough to revert this code back to use >> > find_pc_function. >> > >> > I do have one minor nit though... >> > >> > > if (fn != nullptr && sfn != fn) >> > > { >> > > if (!query (_("Line %d is not in `%s'. Jump anyway? "), sal.line, >> > > diff --git a/gdb/testsuite/gdb.base/jump-inline.c >> > b/gdb/testsuite/gdb.base/jump-inline.c >> > > new file mode 100644 >> > > index 00000000000..17447c2d557 >> > > --- /dev/null >> > > +++ b/gdb/testsuite/gdb.base/jump-inline.c >> > > @@ -0,0 +1,30 @@ >> > > +/* Copyright 2021-2023 Free Software Foundation, Inc. >> > > + >> > > + This program is free software; you can redistribute it and/or modify >> > > + it under the terms of the GNU General Public License as published by >> > > + the Free Software Foundation; either version 2 of the License, or >> > > + (at your option) any later version. >> > > + >> > > + This program is distributed in the hope that it will be useful, >> > > + but WITHOUT ANY WARRANTY; without even the implied warranty of >> > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > > + GNU General Public License for more details. >> > > + >> > > + You should have received a copy of the GNU General Public License >> > > + along with this program. If not, see . >> > */ >> > > + >> > > +__attribute__((always_inline)) >> > > +static void inline >> > > +function_inline (int x) >> > > +{ >> > > + int a = x; >> > > + a += 1020 + a; /* increment-funct. */ >> > > + a = a + x; /* inline-funct. */ >> > > +} >> > > + >> > > +int >> > > +main () >> > > +{ >> > > + function_inline (510); >> > > + return 0; /* out-of-func. */ >> > > +} >> > > diff --git a/gdb/testsuite/gdb.base/jump-inline.exp >> > b/gdb/testsuite/gdb.base/jump-inline.exp >> > > new file mode 100644 >> > > index 00000000000..fef29fedb2f >> > > --- /dev/null >> > > +++ b/gdb/testsuite/gdb.base/jump-inline.exp >> > > @@ -0,0 +1,45 @@ >> > > +# Copyright 2021-2023 Free Software Foundation, Inc. >> > > + >> > > +# This program is free software; you can redistribute it and/or modify >> > > +# it under the terms of the GNU General Public License as published by >> > > +# the Free Software Foundation; either version 3 of the License, or >> > > +# (at your option) any later version. >> > > +# >> > > +# This program is distributed in the hope that it will be useful, >> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > > +# GNU General Public License for more details. >> > > +# >> > > +# You should have received a copy of the GNU General Public License >> > > +# along with this program. If not, see . >> > */ >> > > +# >> > > +# Tests GDBs support for jump for inline functions. >> > > + >> > > +standard_testfile >> > > + >> > > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { >> > > + return -1 >> > > +} >> > > + >> > > +if { ![runto_main] } { >> > > + untested "failed to run to main" >> > >> > This untested line is not needed. runto_main will emit a FAIL if >> > anything goes wrong. >> >> Right, I will remove it. >> >> > Reviewed-By: Andrew Burgess >> > >> > Thanks, >> > Andrew >> >> Thank you for your review and thorough feedback. I agree with your >> thoughts and conclusion. >> I am wondering though, as you gave me a reviewed-by but not an >> approved-by, should I wait a bit more or can I push this? I posted it originally >> in January and pinged a few times. So I tend to think that waiting more isn't >> worth it, but I am of course fine with doing that. >> >> Thanks, >> Felix > > *Pinging again after 10 days of no further comments* > > Is it okay to push this with the untested line removed? As nobody has complained: Approved-By: Andrew Burgess Thanks, Andrew > > Thanks, > Felix > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928