[PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 134 bytes --]

Hi all,

The current patch is to fix the issue defined in PR 9514. There's no
regression. Please review this.

Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 158 bytes --]

2011-09-13  Abhijit Halder  <abhijit.k.halder@gmail.com>

	Fix PR gdb/9837:
	* c-exp.y (abs_decl): Add new rule to resolve pointer(s) to a
	function pointer.

[-- Attachment #3: gdb-parse-error.patch --]
[-- Type: text/x-patch, Size: 509 bytes --]

Index: gdb/c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.82
diff -a -p -u -r1.82 c-exp.y
--- gdb/c-exp.y	6 May 2011 14:12:17 -0000	1.82
+++ gdb/c-exp.y	29 Sep 2011 10:26:20 -0000
@@ -926,6 +926,8 @@ const_or_volatile_or_space_identifier: 
 
 abs_decl:	'*'
 			{ push_type (tp_pointer); $$ = 0; }
+	|	abs_decl '*'
+			{ push_type (tp_pointer); $$ = $1; }
 	|	'*' abs_decl
 			{ push_type (tp_pointer); $$ = $2; }
 	|	'&'

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

On Thu, Sep 29, 2011 at 4:10 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> Hi all,
>
> The current patch is to fix the issue defined in PR 9514. There's no
> regression. Please review this.
>
> Thanks,
> Abhijit Halder
>

Oops! The ChangeLog is incorrect. Correcting the same.

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 158 bytes --]

2011-09-29  Abhijit Halder  <abhijit.k.halder@gmail.com>

	Fix PR gdb/9514:
	* c-exp.y (abs_decl): Add new rule to resolve pointer(s) to a
	function pointer.

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Pedro Alves]

On Thursday 29 September 2011 11:44:04, Abhijit Halder wrote:
> On Thu, Sep 29, 2011 at 4:10 PM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
> > Hi all,
> >
> > The current patch is to fix the issue defined in PR 9514. There's no
> > regression. Please review this.
> >
> > Thanks,
> > Abhijit Halder
> >
> 
> Oops! The ChangeLog is incorrect. Correcting the same.

Please always post both ChangeLog and patch together.  It's
practically 0 work to repost the whole thing instead of just a
part, while having all pieces together is easier for review, as
it avoids the reviewer, not being as familiar with the patches
as you, having to hunt for the pieces.  Also, please always try
to give explanations of what was wrong in the current code, and how
you're fixing the problem.  If you found a problem with an earlier
patch attempt, it's quite useful to know why that earlier patch didn't
work.  If a reviewer will need to try out a patch and go through
the same debug/thought process you had to go throught when writting
the patch, then it's more likely a patch will go by unreviewed
for longer.  In a nutshell, your job is to make it easy to get an OK.

-- 
Pedro Alves

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Thu, Sep 29, 2011 at 4:40 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Thursday 29 September 2011 11:44:04, Abhijit Halder wrote:
>> On Thu, Sep 29, 2011 at 4:10 PM, Abhijit Halder
>> <abhijit.k.halder@gmail.com> wrote:
>> > Hi all,
>> >
>> > The current patch is to fix the issue defined in PR 9514. There's no
>> > regression. Please review this.
>> >
>> > Thanks,
>> > Abhijit Halder
>> >
>>
>> Oops! The ChangeLog is incorrect. Correcting the same.
>
> Please always post both ChangeLog and patch together.  It's
> practically 0 work to repost the whole thing instead of just a
> part, while having all pieces together is easier for review, as
> it avoids the reviewer, not being as familiar with the patches
> as you, having to hunt for the pieces.  Also, please always try
> to give explanations of what was wrong in the current code, and how
> you're fixing the problem.  If you found a problem with an earlier
> patch attempt, it's quite useful to know why that earlier patch didn't
> work.  If a reviewer will need to try out a patch and go through
> the same debug/thought process you had to go throught when writting
> the patch, then it's more likely a patch will go by unreviewed
> for longer.  In a nutshell, your job is to make it easy to get an OK.
>
> --
> Pedro Alves
>

Yes the got the point.

I am re-submitting the whole thing once again for ease of review.

Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 158 bytes --]

2011-09-29  Abhijit Halder  <abhijit.k.halder@gmail.com>

	Fix PR gdb/9514:
	* c-exp.y (abs_decl): Add new rule to resolve pointer(s) to a
	function pointer.

[-- Attachment #3: gdb-parse-error.patch --]
[-- Type: text/x-patch, Size: 509 bytes --]

Index: gdb/c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.82
diff -a -p -u -r1.82 c-exp.y
--- gdb/c-exp.y	6 May 2011 14:12:17 -0000	1.82
+++ gdb/c-exp.y	29 Sep 2011 10:26:20 -0000
@@ -926,6 +926,8 @@ const_or_volatile_or_space_identifier: 
 
 abs_decl:	'*'
 			{ push_type (tp_pointer); $$ = 0; }
+	|	abs_decl '*'
+			{ push_type (tp_pointer); $$ = $1; }
 	|	'*' abs_decl
 			{ push_type (tp_pointer); $$ = $2; }
 	|	'&'

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Tom Tromey]

>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:

Abhijit> I am re-submitting the whole thing once again for ease of review.

This patch needs a patch to the testsuite as well.  Any simple parser
fix should come with a test, as it is easy to do.

Also, please look at my earlier patch for this bug.  I think it shows
some cases that your patch does not address:

    http://sourceware.org/ml/gdb-patches/2008-08/msg00539.html

Perhaps I ought to simply commit that patch.  I am not sure why I never
have.  What do you think?

Tom

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Keith Seitz]

On 10/03/2011 09:45 AM, Tom Tromey wrote:
> Also, please look at my earlier patch for this bug.  I think it shows
> some cases that your patch does not address:
>
>      http://sourceware.org/ml/gdb-patches/2008-08/msg00539.html
>
> Perhaps I ought to simply commit that patch.  I am not sure why I never
> have.  What do you think?

I've looked over both of these patches. Abhijit's original patch is much 
simpler, but there are still regressions using it w/CVS HEAD:

+FAIL: gdb.base/code-expr.exp: (int ** @code)
+FAIL: gdb.base/cvexpr.exp: (int ** const)

I think Tom's approach is more generic, however more (marginally) 
complicated, but it, too, suffers from some problems. Most specifically, 
the ptr_operator production conflicts with the conversion operator 
production. Consider "ptype &foo::operator char* (void)" from 
cplusfuncs.exp.

We end up in the "OPERATOR ptype" production, but because of the new 
ptr_operator rules, this is parsed as OPERATOR, nonempty_typelist, 
func_mod instead of OPERATOR, nonempty_typelist, '(', nonempty_typelist, ')'

So we end up with "operator char (*" with Tom's patch. I keep thinking 
there must be a way to force the parser through the OPERATOR ptype 
production and then the TYPE_INSTANCE production, but I have not been 
successful. More savvy bison-ers might be able to do it, though. Or 
maybe I'll dedicate some time to this and figure it out.

Maybe some crafty massaging of these three productions will yield a 
"simpler" answer.

Keith

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Abhijit Halder]

On Fri, Oct 7, 2011 at 2:39 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 10/03/2011 09:45 AM, Tom Tromey wrote:
>>
>> Also, please look at my earlier patch for this bug.  I think it shows
>> some cases that your patch does not address:
>>
>>     http://sourceware.org/ml/gdb-patches/2008-08/msg00539.html
>>
>> Perhaps I ought to simply commit that patch.  I am not sure why I never
>> have.  What do you think?
>
> I've looked over both of these patches. Abhijit's original patch is much
> simpler, but there are still regressions using it w/CVS HEAD:
>
> +FAIL: gdb.base/code-expr.exp: (int ** @code)
> +FAIL: gdb.base/cvexpr.exp: (int ** const)
>
With latest CVS code the patch has no regression:

ahalder@ahalder-VirtualBox:~/Projects/gdb-enhanced/sandbox/src/gdb$
make check RUNTESTFLAGS=cvexpr.exp
make[1]: Entering directory
`/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite'
Nothing to be done for all...
rootme=`pwd`; export rootme; srcdir=. ; export srcdir ; EXPECT=`if [
-f ${rootme}/../../expect/expect ] ; then echo
${rootme}/../../expect/expect ; else echo expect ; fi` ; export EXPECT
; EXEEXT= ; export EXEEXT ;
LD_LIBRARY_PATH=$rootme/../../expect:$rootme/../../libstdc++:$rootme/../../tk/unix:$rootme/../../tcl/unix:$rootme/../../bfd:$rootme/../../opcodes:$LD_LIBRARY_PATH;
export LD_LIBRARY_PATH; if [ -f ${rootme}/../../expect/expect ] ; then
TCL_LIBRARY=${srcdir}/../../tcl/library ; export TCL_LIBRARY ; fi ;
runtest cvexpr.exp
Test Run By ahalder on Mon Oct 17 14:48:12 2011
Native configuration is i686-pc-linux-gnu

		=== gdb tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file
for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using ./config/unix.exp as tool-and-target-specific interface file.
Running ./gdb.base/cvexpr.exp ...

		=== gdb Summary ===

# of expected passes		115
/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite/../../gdb/gdb
version  7.3.50.20111017-cvs -nw -nx -data-directory
/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite/../data-directory

make[1]: Leaving directory
`/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite'
ahalder@ahalder-VirtualBox:~/Projects/gdb-enhanced/sandbox/src/gdb$
make check RUNTESTFLAGS=code-expr.exp
make[1]: Entering directory
`/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite'
Nothing to be done for all...
rootme=`pwd`; export rootme; srcdir=. ; export srcdir ; EXPECT=`if [
-f ${rootme}/../../expect/expect ] ; then echo
${rootme}/../../expect/expect ; else echo expect ; fi` ; export EXPECT
; EXEEXT= ; export EXEEXT ;
LD_LIBRARY_PATH=$rootme/../../expect:$rootme/../../libstdc++:$rootme/../../tk/unix:$rootme/../../tcl/unix:$rootme/../../bfd:$rootme/../../opcodes:$LD_LIBRARY_PATH;
export LD_LIBRARY_PATH; if [ -f ${rootme}/../../expect/expect ] ; then
TCL_LIBRARY=${srcdir}/../../tcl/library ; export TCL_LIBRARY ; fi ;
runtest code-expr.exp
Test Run By ahalder on Mon Oct 17 14:48:19 2011
Native configuration is i686-pc-linux-gnu

		=== gdb tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file
for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using ./config/unix.exp as tool-and-target-specific interface file.
Running ./gdb.base/code-expr.exp ...

		=== gdb Summary ===

# of expected passes		101
/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite/../../gdb/gdb
version  7.3.50.20111017-cvs -nw -nx -data-directory
/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite/../data-directory

make[1]: Leaving directory
`/home/ahalder/Projects/gdb-enhanced/sandbox/src/gdb/testsuite'
ahalder@ahalder-VirtualBox:~/Projects/gdb-enhanced/sandbox/src/gdb$

------------------------------------------------------------------------------------------------------------------

ahalder@ahalder-VirtualBox:~/Projects/gdb-enhanced/sandbox/src/gdb$
cvsdiff c-exp.y
Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.84
diff -a -p -u -r1.84 c-exp.y
--- c-exp.y	11 Oct 2011 15:24:10 -0000	1.84
+++ c-exp.y	17 Oct 2011 10:58:06 -0000
@@ -942,6 +942,8 @@ const_or_volatile_or_space_identifier:

 abs_decl:	'*'
 			{ push_type (tp_pointer); $$ = 0; }
+	|	abs_decl '*'
+			{ push_type (tp_pointer); $$ = $1; }
 	|	'*' abs_decl
 			{ push_type (tp_pointer); $$ = $2; }
 	|	'&'


> I think Tom's approach is more generic, however more (marginally)
> complicated, but it, too, suffers from some problems. Most specifically, the
> ptr_operator production conflicts with the conversion operator production.
> Consider "ptype &foo::operator char* (void)" from cplusfuncs.exp.
>
> We end up in the "OPERATOR ptype" production, but because of the new
> ptr_operator rules, this is parsed as OPERATOR, nonempty_typelist, func_mod
> instead of OPERATOR, nonempty_typelist, '(', nonempty_typelist, ')'
>
> So we end up with "operator char (*" with Tom's patch. I keep thinking there
> must be a way to force the parser through the OPERATOR ptype production and
> then the TYPE_INSTANCE production, but I have not been successful. More
> savvy bison-ers might be able to do it, though. Or maybe I'll dedicate some
> time to this and figure it out.
>
> Maybe some crafty massaging of these three productions will yield a
> "simpler" answer.
>
> Keith
>

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Keith Seitz]

On 10/17/2011 03:59 AM, Abhijit Halder wrote:
>> +FAIL: gdb.base/code-expr.exp: (int ** @code)
>> +FAIL: gdb.base/cvexpr.exp: (int ** const)
>>
> With latest CVS code the patch has no regression:

I still see those as regressions (gdb 7.3.50.20111018-cvs):

$ diff -p testsuite/gdb.sum.0 testsuite/gdb.sum | grep "int \*\*"
   PASS: gdb.base/code-expr.exp: (@code int **)
! PASS: gdb.base/code-expr.exp: (int ** @code)
   PASS: gdb.base/code-expr.exp: (@code int **)
! FAIL: gdb.base/code-expr.exp: (int ** @code)
   PASS: gdb.base/cvexpr.exp: (const int **)
! PASS: gdb.base/cvexpr.exp: (int ** const)
   PASS: gdb.base/cvexpr.exp: (const int **)
! FAIL: gdb.base/cvexpr.exp: (int ** const)

Do I have an outdated version of the patch?

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index b850179..60004f3 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -944,6 +944,8 @@ abs_decl:   '*'
                         { push_type (tp_pointer); $$ = 0; }
         |       '*' abs_decl
                         { push_type (tp_pointer); $$ = $2; }
+       |       abs_decl '*'
+                       { push_type (tp_pointer); $$ = $1; }
         |       '&'
                         { push_type (tp_reference); $$ = 0; }
         |       '&' abs_decl


Am I missing something else?

Keith

Re: [PATCH] [PR 9514] Fixing parse error for "pointer to a function pointer"

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> I still see those as regressions (gdb 7.3.50.20111018-cvs):

Also make sure to run the patch against the tests included in my patch.
My recollection is that I originally had a much simpler patch but that
the 'const' cases made it fail.

Tom