Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273)

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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)<TAB> or 'outer::foo if (a == 3)<ESC-?>
	(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