From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20386 invoked by alias); 27 Feb 2011 21:16:50 -0000 Received: (qmail 20378 invoked by uid 22791); 27 Feb 2011 21:16:49 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL,BAYES_00,FAKE_REPLY_C,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 27 Feb 2011 21:16:42 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p1RLGe5C001358 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 27 Feb 2011 16:16:40 -0500 Received: from host1.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p1RLGcZL028750 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 27 Feb 2011 16:16:40 -0500 Received: from host1.dyn.jankratochvil.net (localhost [127.0.0.1]) by host1.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p1RLGc5Y020317; Sun, 27 Feb 2011 22:16:38 +0100 Received: (from jkratoch@localhost) by host1.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id p1RLGbgc020316; Sun, 27 Feb 2011 22:16:37 +0100 Date: Sun, 27 Feb 2011 21:18:00 -0000 From: Jan Kratochvil To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273) Message-ID: <20110227211637.GA18378@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D66C244.5020005@redhat.com> <20110221110627.GA29540@host1.dyn.jankratochvil.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 X-SW-Source: 2011-02/txt/msg00843.txt.bz2 On Thu, 24 Feb 2011 21:40:36 +0100, Keith Seitz wrote: > On 02/21/2011 03:06 AM, Jan Kratochvil wrote: > >pre-physname: > >GNU gdb (GDB) 7.1.50.20100309-cvs > >(gdb) b A::outer::foo (int) > >Breakpoint 1 at 0x4005a3: file ./gdb.cp/ovsrch3.cc, line 23. > > > >patched: > >GNU gdb (GDB) 7.2.50.20110220-cvs > >(gdb) b A::outer::foo (int) > >the class A::outer does not have any method named foo (int) > > This is caused by one of the consequences of dwarf2_physname: we > need to have all user input canonicalized before calling > lookup_symbol[*]. This is a problem I have wondered about since the > dwarf2_physname patchset was originally committed, and now we have a > concrete example of how to trigger this. This part is now fixed, thanks. > If I have understood you correctly, I've removed the quote character > and tested independently for it using > get_gdb_completer_quote_characters. Yes, I find this change as one of the valid possibilites. > >00000000004004ea W _ZNK1C1mEv > >00000000004004ea W C::m() const > > > >pre-physname: > >GNU gdb (GDB) 7.1.50.20100309-cvs > >(gdb) b C::m if C::m() > >Breakpoint 1 at 0x4004f2: file 3.C, line 4. > >(gdb) b 'C::m' if C::m() > >Breakpoint 1 at 0x4004f2: file 3.C, line 4. > > > >patched: > >GNU gdb (GDB) 7.2.50.20110220-cvs > >(gdb) b C::m if C::m() > >the class C does not have any method named m if C::m() > >(gdb) b 'C::m' if C::m() > >the class C does not have any method named m' if C::m() > > This is also fixed, This is still not fixed. I quoted there the line: > > + char *paren = strchr (p, '('); The problem is it skips anything in between, incl. the " if " keyword. (There are two cases of this quoted GDB line of code.) Maybe some - simplified while (isspace (*p)) p++; if (*p == '(') would be enough? I did not try. > and I have added a test for this to ovsrch.exp. The testcase still does not use a bare method name without any parentheses to reproduce this problem. Such a reproducer turning PASS->FAIL from pre-physname is: echo 'class C { public: void m() const {} } c; int main () { c.m(); }' | gcc -x c++ - -Wall -g; ../gdb -nx ./a.out -ex 'b C::m if C::m()' -ex q It is currently more difficult to see the regression on the gdb.cp/ovsrch.exp testcase with pre-physname GDB as gdb.cp/ovsrch.exp uses namespaces now which were not supported by pre-physname GDB. Contrary to it this minimal testcase above is compatible with pre-physname GDB (that is 42284fdf9d8cdb20c8e833bdbdb2b56977fea525^) > >00000000004004ea W _ZNK1C1mEv > >00000000004004ea W C::m() const > > > >pre-physname: > >GNU gdb (GDB) 7.1.50.20100309-cvs > >(gdb) b 'C::m() const' > >Breakpoint 1 at 0x4004f2: file 3.C, line 4. > > > >patched: > >GNU gdb (GDB) 7.2.50.20110220-cvs > >(gdb) b 'C::m() const' > >Junk at end of arguments. > > > >This is a regression. > > This is also a consequence of (the lack of) canonicalization. I've > modified ovsrch.exp et al to test for this. This is also not fixed. Reproducible by modifying the testcase: - set method "${class}::foo ($ovld) const" + set method "${class}::foo ($ovld) const" The problem is the quoted code: > > + /* Make sure we keep important kewords like "const" */ > > + if (strncmp (p, " const", 6) == 0) > > + p += 6; is executed before find_methods is called at all. Therefore find_methods will get the "const" suffix already stripped. Maybe some - simplified while (isspace (*p)) p++; if (strncmp (p, "const", 5) == 0) would be enough? Also I am suspicious on not checking that !isalnum (p[strlen ("const")]) - that there is a word break and isn't there something like: (gdb) break method(int) constvariablename But I think if there is any string afterwards the caller will abort on it anyway so it is not a problem is part of it gets incorrectly interpreted as the "const" keyword. > I'm not sure whether it would be easier to diff the old and new > patchsets or just attach the whole thing. But I'm erring on the side > of "too much information." I do not mind either way as I commit it first into GIT to operate it. When we already talk about it it would be most convenient to format patches by `git format-patch' so that one can use `git am'. It would also better match my battalion of regression testing scripts expecting -p1 patches. > + set conditional1 "if (a == 3)" > + set conditional2 "if (A::outer::func ())" > + foreach ovld [array names tests] { > + set method "${class}::foo ($ovld) const" Here isn't tested: set method "${class}::foo" which with patched GDB will: (gdb) break outer::foo Breakpoint 4 at 0x4005c0: file ./gdb.cp/ovsrch4.cc, line 23. PASS: gdb.cp/ovsrch.exp: break outer::foo but with patched GDB it will also: (gdb) break outer::foo if (a == 3) the class A::outer does not have any method named foo if (a == 3) Hint: try 'outer::foo if (a == 3) or 'outer::foo if (a == 3) (Note leading single quote.) Make breakpoint pending on future shared library load? (y or [n]) n FAIL: gdb.cp/ovsrch.exp: break outer::foo if (a == 3) (got interactive prompt) Thanks, Jan