Re: [PATCH] Handle OP_STRING in dump_subexp_body_standard

[Simon Marchi <simon dot marchi at ericsson dot com>]

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