diff mbox

Handle OP_STRING in dump_subexp_body_standard

Message ID 1403189133-7667-1-git-send-email-simon.marchi@ericsson.com
State Committed
Headers

Commit Message

Simon Marchi June 19, 2014, 2:45 p.m. UTC 
  For some reason, OP_STRING is not handled in dump_subexp_body_standard.
This makes the output of "set debug expression 1" very bad when a string
is involved. Example:

(gdb) set debug expression 1
(gdb) print "hello"
... (random garbage, possibly segfault)

This commit handles OP_STRING and skips the appropriate number of exp
elements. The line corresponding to the string now looks like:

	    0  OP_STRING             Language-specific string type: 0

gdb/ChangeLog:

2014-06-19  Simon Marchi  <simon.marchi@ericsson.com>

	* expprint.c (dump_subexp_body_standard): Handle OP_STRING.
---
 gdb/expprint.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Simon Marchi July 8, 2014, 3:28 p.m. UTC | #1 
On 14-06-19 10:45 AM, Simon Marchi wrote:
> For some reason, OP_STRING is not handled in dump_subexp_body_standard.
> This makes the output of "set debug expression 1" very bad when a string
> is involved. Example:
> 
> (gdb) set debug expression 1
> (gdb) print "hello"
> ... (random garbage, possibly segfault)
> 
> This commit handles OP_STRING and skips the appropriate number of exp
> elements. The line corresponding to the string now looks like:
> 
> 	    0  OP_STRING             Language-specific string type: 0
> 
> gdb/ChangeLog:
> 
> 2014-06-19  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 	* expprint.c (dump_subexp_body_standard): Handle OP_STRING.
> ---
>  gdb/expprint.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/expprint.c b/gdb/expprint.c
> index 97188ed..60971a5 100644
> --- a/gdb/expprint.c
> +++ b/gdb/expprint.c
> @@ -1011,12 +1011,29 @@ dump_subexp_body_standard (struct expression *exp,
>  	elt = dump_subexp (exp, stream, elt);
>        }
>        break;
> +    case OP_STRING:
> +      {
> +	LONGEST len = exp->elts[elt].longconst;
> +	LONGEST type = exp->elts[elt].longconst;
> +
> +	fprintf_filtered (stream, "Language-specific string type: %s",
> +			  plongest (type));
> +
> +	/* Skip length.  */
> +	elt += 1;
> +
> +	/* Skip string content. */
> +	elt += BYTES_TO_EXP_ELEM(len);
> +
> +	/* Skip length and ending OP_STRING. */
> +	elt += 2;
> +      }
> +      break;
>      default:
>      case OP_NULL:
>      case MULTI_SUBSCRIPT:
>      case OP_F77_UNDETERMINED_ARGLIST:
>      case OP_COMPLEX:
> -    case OP_STRING:
>      case OP_BOOL:
>      case OP_M2_STRING:
>      case OP_THIS:

Ping!
Joel Brobecker July 15, 2014, 1:20 p.m. UTC | #2 
Hello Simon,

> > For some reason, OP_STRING is not handled in dump_subexp_body_standard.
> > This makes the output of "set debug expression 1" very bad when a string
> > is involved. Example:
> > 
> > (gdb) set debug expression 1
> > (gdb) print "hello"
> > ... (random garbage, possibly segfault)
> > 
> > This commit handles OP_STRING and skips the appropriate number of exp
> > elements. The line corresponding to the string now looks like:
> > 
> > 	    0  OP_STRING             Language-specific string type: 0
> > 
> > gdb/ChangeLog:
> > 
> > 2014-06-19  Simon Marchi  <simon.marchi@ericsson.com>
> > 
> > 	* expprint.c (dump_subexp_body_standard): Handle OP_STRING.

Sorry about the delay.

> > ---
> >  gdb/expprint.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/expprint.c b/gdb/expprint.c
> > index 97188ed..60971a5 100644
> > --- a/gdb/expprint.c
> > +++ b/gdb/expprint.c
> > @@ -1011,12 +1011,29 @@ dump_subexp_body_standard (struct expression *exp,
> >  	elt = dump_subexp (exp, stream, elt);
> >        }
> >        break;
> > +    case OP_STRING:
> > +      {
> > +	LONGEST len = exp->elts[elt].longconst;
> > +	LONGEST type = exp->elts[elt].longconst;

These two are the same :-).

Looking at parse.c::write_exp_string_vector, which seems to be
the function responsible for creating OP_STRING nodes, it writes
the opcode, then the length, then the type, and finally the vector:

  write_exp_elt_opcode (ps, OP_STRING);
  write_exp_elt_longcst (ps, len);
  write_exp_elt_longcst (ps, type);
  [loop writing the vector]

So, I would say that it should be "elt + 1" for variable "type".

> > +	fprintf_filtered (stream, "Language-specific string type: %s",
> > +			  plongest (type));
> > +
> > +	/* Skip length.  */
> > +	elt += 1;
> > +
> > +	/* Skip string content. */
> > +	elt += BYTES_TO_EXP_ELEM(len);

Missing space before '('.
Simon Marchi July 15, 2014, 2:45 p.m. UTC | #3 
Hi Joel,

On 14-07-15 09:20 AM, Joel Brobecker wrote:
> Hello Simon,
> 
>>> For some reason, OP_STRING is not handled in dump_subexp_body_standard.
>>> This makes the output of "set debug expression 1" very bad when a string
>>> is involved. Example:
>>>
>>> (gdb) set debug expression 1
>>> (gdb) print "hello"
>>> ... (random garbage, possibly segfault)
>>>
>>> This commit handles OP_STRING and skips the appropriate number of exp
>>> elements. The line corresponding to the string now looks like:
>>>
>>> 	    0  OP_STRING             Language-specific string type: 0
>>>
>>> gdb/ChangeLog:
>>>
>>> 2014-06-19  Simon Marchi  <simon.marchi@ericsson.com>
>>>
>>> 	* expprint.c (dump_subexp_body_standard): Handle OP_STRING.
> 
> Sorry about the delay.
> 
>>> ---
>>>  gdb/expprint.c | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/expprint.c b/gdb/expprint.c
>>> index 97188ed..60971a5 100644
>>> --- a/gdb/expprint.c
>>> +++ b/gdb/expprint.c
>>> @@ -1011,12 +1011,29 @@ dump_subexp_body_standard (struct expression *exp,
>>>  	elt = dump_subexp (exp, stream, elt);
>>>        }
>>>        break;
>>> +    case OP_STRING:
>>> +      {
>>> +	LONGEST len = exp->elts[elt].longconst;
>>> +	LONGEST type = exp->elts[elt].longconst;
> 
> These two are the same :-).
> 
> Looking at parse.c::write_exp_string_vector, which seems to be
> the function responsible for creating OP_STRING nodes, it writes
> the opcode, then the length, then the type, and finally the vector:
> 
>   write_exp_elt_opcode (ps, OP_STRING);
>   write_exp_elt_longcst (ps, len);
>   write_exp_elt_longcst (ps, type);
>   [loop writing the vector]
> 
> So, I would say that it should be "elt + 1" for variable "type".

You are totally right.

>>> +	fprintf_filtered (stream, "Language-specific string type: %s",
>>> +			  plongest (type));
>>> +
>>> +	/* Skip length.  */
>>> +	elt += 1;
>>> +
>>> +	/* Skip string content. */
>>> +	elt += BYTES_TO_EXP_ELEM(len);
> 
> Missing space before '('.

Ack.

Thanks for the review. Do these small fixes warrant a v2?

Simon
Joel Brobecker July 15, 2014, 3 p.m. UTC | #4 
> >>> gdb/ChangeLog:
> >>>
> >>> 2014-06-19  Simon Marchi  <simon.marchi@ericsson.com>
> >>>
> >>> 	* expprint.c (dump_subexp_body_standard): Handle OP_STRING.
[...]
> > So, I would say that it should be "elt + 1" for variable "type".
> 
> You are totally right.
> 
> >>> +	/* Skip string content. */
> >>> +	elt += BYTES_TO_EXP_ELEM(len);
> > 
> > Missing space before '('.
> 
> Ack.
> 
> Thanks for the review. Do these small fixes warrant a v2?

OK, pre-approved with the changes above, but our procedures do require
you to re-post a patch whenever what's committed is different from
what was originally posted. I'd say, make the modifications, test them,
commit & push, and then reply to this thread with the updated patch.

Thank you,
diff mbox

Patch

diff --git a/gdb/expprint.c b/gdb/expprint.c
index 97188ed..60971a5 100644
--- a/gdb/expprint.c
+++ b/gdb/expprint.c
@@ -1011,12 +1011,29 @@  dump_subexp_body_standard (struct expression *exp,
 	elt = dump_subexp (exp, stream, elt);
       }
       break;
+    case OP_STRING:
+      {
+	LONGEST len = exp->elts[elt].longconst;
+	LONGEST type = exp->elts[elt].longconst;
+
+	fprintf_filtered (stream, "Language-specific string type: %s",
+			  plongest (type));
+
+	/* Skip length.  */
+	elt += 1;
+
+	/* Skip string content. */
+	elt += BYTES_TO_EXP_ELEM(len);
+
+	/* Skip length and ending OP_STRING. */
+	elt += 2;
+      }
+      break;
     default:
     case OP_NULL:
     case MULTI_SUBSCRIPT:
     case OP_F77_UNDETERMINED_ARGLIST:
     case OP_COMPLEX:
-    case OP_STRING:
     case OP_BOOL:
     case OP_M2_STRING:
     case OP_THIS: