RFC: fix PR symtab/11464

RFC: fix PR symtab/11464

[Tom Tromey]

This patch fixes PR symtab/11464.

The bug is that using a quoted destructor in an expression crashes gdb.
The example from the PR is: ptype gnu_obj_1::'~gnu_obj_1'
This is odd, but of course shouldn't crash.

The basic problem was that when yylex and classify_inner_name were
updated to handle NAME in addition to TYPENAME, nothing was done to
ensure that the value of yylval seen by classify_inner_name actually had
a valid 'tsym.type' field.  And, in this case, it doesn't.

The patch fixes this by making some state more explicit.  I think this
is a plus -- really the whole lexer could use a cleanup along these
lines, removing most uses of yylval, ensuring that the proper union
fields are always used, etc.  But, I didn't go that far.

Built and regtested on x86-64 Fedora 16.
A new test case, from the PR, is included -- I didn't include the full
patch from the PR as it also covers a couple of unrelated bugs.

Tom

2013-01-10  Tom Tromey  <tromey@redhat.com>

	PR symtab/11464:
	* c-exp.y (lex_one_token): Initialize other fields of yylval on
	NAME return.
	(classify_inner_name): Remove 'first_name' argument, add
	'context'.  Remove unused variable.
	(yylex): Explicitly maintain the context type.  Exit loop earlier
	if NAME result is seen.

2013-01-10  Tom Tromey  <tromey@redhat.com>

	* gdb.cp/m-static.cc (gnu_obj_1::~gnu_obj_1): New destructor.
	* gdb.cp/m-static.exp: Add tests to print quoted destructor.

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 9847120..9ef4b39 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -2719,6 +2719,10 @@ lex_one_token (void)
 
   if (parse_completion && *lexptr == '\0')
     saw_name_at_eof = 1;
+
+  yylval.ssym.stoken = yylval.sval;
+  yylval.ssym.sym = NULL;
+  yylval.ssym.is_a_field_of_this = 0;
   return NAME;
 }
 
@@ -2859,26 +2863,26 @@ classify_name (const struct block *block)
 }
 
 /* Like classify_name, but used by the inner loop of the lexer, when a
-   name might have already been seen.  FIRST_NAME is true if the token
-   in `yylval' is the first component of a name, false otherwise.  */
+   name might have already been seen.  CONTEXT is the context type, or
+   NULL if this is the first component of a name.  */
 
 static int
-classify_inner_name (const struct block *block, int first_name)
+classify_inner_name (const struct block *block, struct type *context)
 {
-  struct type *type, *new_type;
+  struct type *type;
   char *copy;
 
-  if (first_name)
+  if (context == NULL)
     return classify_name (block);
 
-  type = check_typedef (yylval.tsym.type);
+  type = check_typedef (context);
   if (TYPE_CODE (type) != TYPE_CODE_STRUCT
       && TYPE_CODE (type) != TYPE_CODE_UNION
       && TYPE_CODE (type) != TYPE_CODE_NAMESPACE)
     return ERROR;
 
-  copy = copy_name (yylval.tsym.stoken);
-  yylval.ssym.sym = cp_lookup_nested_symbol (yylval.tsym.type, copy, block);
+  copy = copy_name (yylval.ssym.stoken);
+  yylval.ssym.sym = cp_lookup_nested_symbol (type, copy, block);
   if (yylval.ssym.sym == NULL)
     return ERROR;
 
@@ -2893,7 +2897,6 @@ classify_inner_name (const struct block *block, int first_name)
       return TYPENAME;
 
     default:
-      yylval.ssym.is_a_field_of_this = 0;
       return NAME;
     }
   internal_error (__FILE__, __LINE__, _("not reached"));
@@ -2915,6 +2918,7 @@ yylex (void)
 {
   token_and_value current;
   int first_was_coloncolon, last_was_coloncolon, first_iter;
+  struct type *context_type = NULL;
 
   if (popping && !VEC_empty (token_and_value, token_fifo))
     {
@@ -2936,7 +2940,10 @@ yylex (void)
   last_was_coloncolon = first_was_coloncolon;
   obstack_free (&name_obstack, obstack_base (&name_obstack));
   if (!last_was_coloncolon)
-    obstack_grow (&name_obstack, yylval.sval.ptr, yylval.sval.length);
+    {
+      obstack_grow (&name_obstack, yylval.sval.ptr, yylval.sval.length);
+      context_type = yylval.tsym.type;
+    }
   current.value = yylval;
   first_iter = 1;
   while (1)
@@ -2953,7 +2960,7 @@ yylex (void)
 	  classification = classify_inner_name (first_was_coloncolon
 						? NULL
 						: expression_context_block,
-						first_iter);
+						context_type);
 	  /* We keep going until we either run out of names, or until
 	     we have a qualified name which is not a type.  */
 	  if (classification != TYPENAME && classification != NAME)
@@ -2964,7 +2971,7 @@ yylex (void)
 	    }
 
 	  /* Update the partial name we are constructing.  */
-	  if (!first_iter)
+	  if (context_type != NULL)
 	    {
 	      /* We don't want to put a leading "::" into the name.  */
 	      obstack_grow_str (&name_obstack, "::");
@@ -2978,6 +2985,11 @@ yylex (void)
 	  current.token = classification;
 
 	  last_was_coloncolon = 0;
+	  
+	  if (classification == NAME)
+	    break;
+
+	  context_type = yylval.tsym.type;
 	}
       else if (next.token == COLONCOLON && !last_was_coloncolon)
 	last_was_coloncolon = 1;
diff --git a/gdb/testsuite/gdb.cp/m-static.cc b/gdb/testsuite/gdb.cp/m-static.cc
index e9dce18..8472988 100644
--- a/gdb/testsuite/gdb.cp/m-static.cc
+++ b/gdb/testsuite/gdb.cp/m-static.cc
@@ -17,6 +17,7 @@ protected:
 
 public:
   gnu_obj_1(antiquities a, long l) {}
+  ~gnu_obj_1() {}
 
   long method ()
   {
diff --git a/gdb/testsuite/gdb.cp/m-static.exp b/gdb/testsuite/gdb.cp/m-static.exp
index 38d2498..ae4b2ad 100644
--- a/gdb/testsuite/gdb.cp/m-static.exp
+++ b/gdb/testsuite/gdb.cp/m-static.exp
@@ -64,6 +64,14 @@ gdb_test "print test1.key2" "\\$\[0-9\]* = 77" "simple object, static long"
 # simple object, static enum
 gdb_test "print test1.value" "\\$\[0-9\]* = oriental" "simple object, static enum"
 
+gdb_test "print test1.'~gnu_obj_1'" \
+    { = {void \(gnu_obj_1 \*( const)?, int\)} 0x[0-9a-f]+ <gnu_obj_1::~gnu_obj_1\(\)>} \
+    "simple object instance, print quoted destructor"
+
+gdb_test "ptype gnu_obj_1::'~gnu_obj_1'" \
+    {type = void \(gnu_obj_1 \* const\)} \
+    "simple object class, ptype quoted destructor"
+
 # Two.
 
 # derived template object, base static const bool

Re: RFC: fix PR symtab/11464

[Jan Kratochvil]

On Thu, 10 Jan 2013 20:50:56 +0100, Tom Tromey wrote:
> 2013-01-10  Tom Tromey  <tromey@redhat.com>
> 
> 	PR symtab/11464:
> 	* c-exp.y (lex_one_token): Initialize other fields of yylval on
> 	NAME return.
> 	(classify_inner_name): Remove 'first_name' argument, add
> 	'context'.  Remove unused variable.
> 	(yylex): Explicitly maintain the context type.  Exit loop earlier
> 	if NAME result is seen.
> 
> 2013-01-10  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.cp/m-static.cc (gnu_obj_1::~gnu_obj_1): New destructor.
> 	* gdb.cp/m-static.exp: Add tests to print quoted destructor.

I do not see there a regression; this does not mean much for c-exp.y.

> 
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index 9847120..9ef4b39 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -2978,6 +2985,11 @@ yylex (void)
>  	  current.token = classification;
>  
>  	  last_was_coloncolon = 0;
> +	  
> +	  if (classification == NAME)
> +	    break;

IIUC this is a parsing cleanup unrelated to this bug.


> +
> +	  context_type = yylval.tsym.type;
>  	}
>        else if (next.token == COLONCOLON && !last_was_coloncolon)
>  	last_was_coloncolon = 1;



Thanks,
Jan

Re: RFC: fix PR symtab/11464

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I do not see there a regression; this does not mean much for c-exp.y.

I'll look.
It may be more visible under valgrind.

>> +	  
>> +	  if (classification == NAME)
>> +	    break;

Jan> IIUC this is a parsing cleanup unrelated to this bug.

No, I think we have to only allow TYPENAME results to continue the loop.
This is part of the core of the bug - that we would end up trying to use
yylval.tsym.type when it was not valid.

Tom

Re: RFC: fix PR symtab/11464

[Tom Tromey]

Jan> I do not see there a regression; this does not mean much for c-exp.y.

Tom> I'll look.
Tom> It may be more visible under valgrind.

For me it reliably fails the 'ptype':

(gdb) ptype gnu_obj_1::'~gnu_obj_1'
Segmentation fault (core dumped)

Tom

Re: RFC: fix PR symtab/11464

[Jan Kratochvil]

On Mon, 04 Feb 2013 22:47:05 +0100, Tom Tromey wrote:
> Jan> I do not see there a regression; this does not mean much for c-exp.y.
> 
> Tom> I'll look.
> Tom> It may be more visible under valgrind.
> 
> For me it reliably fails the 'ptype':
> 
> (gdb) ptype gnu_obj_1::'~gnu_obj_1'
> Segmentation fault (core dumped)

I really have the full testsuite without regressions.  I do not see why it
crashes for you.  Tried on Fedora Rawhide, both with -lmcheck and without
-lmcheck.

This case gnu_obj_1::'~gnu_obj_1' does not get handled there as the parser
parses it as:
   $1 = token TYPENAME ()
   $2 = token COLONCOLON ()
   $3 = nterm name ()
and when it sees '~gnu_obj_1' classify_inner_name returns ERROR.

Without these two lines:
        if (classification == NAME)
          break;
the loop goes again, last_was_coloncolon == 0 in
      if (next.token == NAME && last_was_coloncolon)
So it just stores back the fetched next token
          /* We've reached the end of the name.  */
          VEC_safe_push (token_and_value, token_fifo, &next);
and also does "break;".

So I still believe those two lines
        if (classification == NAME)
          break;
are just some cleanup/simplification but not required by this patch.


Thanks,
Jan

Re: RFC: fix PR symtab/11464

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I really have the full testsuite without regressions.  I do not see why it
Jan> crashes for you.  Tried on Fedora Rawhide, both with -lmcheck and without
Jan> -lmcheck.

This is quite strange.  Today I rebased this patch onto gdb/master.
Then I built gdb without the patch and ran the test case that was
included in the patch.  In gdb.log I then see:

    ptype gnu_obj_1::'~gnu_obj_1'
    ERROR: Process no longer exists

Jan> So I still believe those two lines
Jan>         if (classification == NAME)
Jan>           break;
Jan> are just some cleanup/simplification but not required by this patch.

Ok, I see what you mean, but the subsequent line is not valid for NAME:

	  context_type = yylval.tsym.type;

I guess you are saying I should invert the if.
Ok, I will do that.

Tom

Re: RFC: fix PR symtab/11464

[Jan Kratochvil]

On Mon, 11 Feb 2013 21:28:05 +0100, Tom Tromey wrote:
> This is quite strange.  Today I rebased this patch onto gdb/master.
> Then I built gdb without the patch and ran the test case that was
> included in the patch.  In gdb.log I then see:
> 
>     ptype gnu_obj_1::'~gnu_obj_1'
>     ERROR: Process no longer exists

That's true, that crashes even for me.  I still expected it crashes for you if
you remove the two lines:
         if (classification == NAME)
           break;
which cannot / does not crash.


> Jan> So I still believe those two lines
> Jan>         if (classification == NAME)
> Jan>           break;
> Jan> are just some cleanup/simplification but not required by this patch.
> 
> Ok, I see what you mean, but the subsequent line is not valid for NAME:
> 
> 	  context_type = yylval.tsym.type;

OK, that makes sense now.

(It cannot crash but I agree it is not clean to read such data.)


> I guess you are saying I should invert the if.
> Ok, I will do that.

No, I did not mean that.  In fact I do not understand much what condition
inversion do you mean.

I am fine with the patch now.


Thanks,
Jan

Re: RFC: fix PR symtab/11464

[Tom Tromey]

Jan> That's true, that crashes even for me.  I still expected it crashes
Jan> for you if you remove the two lines:
Jan>          if (classification == NAME)
Jan>            break;
Jan> which cannot / does not crash.

I thought you were reporting that the test case did not crash before the
fix.

Now I don't understand what you are saying here.

>> I guess you are saying I should invert the if.
>> Ok, I will do that.

Jan> No, I did not mean that.  In fact I do not understand much what condition
Jan> inversion do you mean.

I mean now the patch reads:

	  if (classification != NAME)
	    context_type = yylval.tsym.type;

This still fixes the failing test case, but I haven't run it through a
complete regression test yet.

Tom

Re: RFC: fix PR symtab/11464

[Jan Kratochvil]

On Mon, 11 Feb 2013 21:45:01 +0100, Tom Tromey wrote:
> Jan> That's true, that crashes even for me.  I still expected it crashes
> Jan> for you if you remove the two lines:
> Jan>          if (classification == NAME)
> Jan>            break;
> Jan> which cannot / does not crash.
> 
> I thought you were reporting that the test case did not crash before the
> fix.
> 
> Now I don't understand what you are saying here.

It does not matter but the whole time I was trying to say this does not crash
for me:

--- ./gdb/c-exp.y-x	2013-02-11 21:53:30.837153125 +0100
+++ ./gdb/c-exp.y	2013-02-11 21:53:44.947170669 +0100
@@ -2986,8 +2986,8 @@ yylex (void)
 
 	  last_was_coloncolon = 0;
 	  
-	  if (classification == NAME)
-	    break;
+//	  if (classification == NAME)
+//	    break;
 
 	  context_type = yylval.tsym.type;
 	}


> >> I guess you are saying I should invert the if.
> >> Ok, I will do that.
> 
> Jan> No, I did not mean that.  In fact I do not understand much what condition
> Jan> inversion do you mean.
> 
> I mean now the patch reads:
> 
> 	  if (classification != NAME)
> 	    context_type = yylval.tsym.type;
> 
> This still fixes the failing test case, but I haven't run it through a
> complete regression test yet.

In such case there should be:

	  if (classification != NAME)
	    context_type = yylval.tsym.type;
	  else
	    context_type = NULL;

Otherwise context_type could have "stale" content.

But it has no effect on the executed code.


I do not mind the patch form anymore, it was all just about the crashing /
non-crashing misunderstanding.


Thanks,
Jan

Re: RFC: fix PR symtab/11464

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I do not mind the patch form anymore, it was all just about the crashing /
Jan> non-crashing misunderstanding.

Ok, I am checking in the original then.

Tom