Re: [PATCH] PR mi/15806: Fix quoting of async events

[Simon Marchi <simon.marchi@ericsson.com>]

On 14-05-16 11:42 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> The quoting in whatever goes in the event_channel of MI is little
> Simon> bit broken.  Link for the lazy:
> Simon>   https://sourceware.org/bugzilla/show_bug.cgi?id=15806
> 
> Thanks for looking at this.
> 
> A long time ago I wrote a somewhat similar patch, but I don't think I
> noticed some of the issues you dug up.  Mine was here, I don't recall
> why it didn't go in:
> 
>     https://www.sourceware.org/ml/gdb-patches/2011-01/msg00518.html
> 
> Simon> In retrospect, there is no way escaping can be done reliably in
> Simon> mi_console_raw_packet for data that is already formatted, such as
> Simon> event_channel. At this point, there is no way to differentiate
> Simon> quotes that delimit field values from those that should be
> Simon> escaped. In the case of other MI consoles, it is ok since
> Simon> mi_console_raw_packet receives one big string that should be
> Simon> quoted and escaped as a whole.
> 
> Simon> So, first part of the fix: for the MI channels that specify no
> Simon> quoting character, no escaping at all should be done in
> Simon> mi_console_raw_packet (that's the change in printchar, thanks to
> Simon> Yuanhui Zhang for this).
> 
> Seems reasonable.
> 
> Did you check the other callers of printchar, even indirect ones?
> I did a spot check and didn't see any issues, but it would be reassuring
> if you could look.

The only direct callers of printchar are fputstr_filtered, fputstr_unfiltered, fputstrn_filtered and fputstrn_unfiltered. To the best of my knowledge, the change should not impact the callers of those.

Semi-related: while looking at that, I realized that unlike mi_field_string, mi_field_fmt does no escaping at all. It doesn't seem like a problem right now, since mi_field_fmt is only used with things that don't really need escaping anyway (numbers, or hex strings). Still, it makes an inconsistent API and would be nice to fix, I think.

> Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>
> Simon> 	PR mi/15806
> Simon> 	* utils.c (printchar): Don't escape at all if quoter is NUL.
> Simon> 	* mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
> Simon> 	generate the output.
> Simon> 	(mi_solib_unloaded): Same.
> 
> Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>
> 
> Simon> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
> Simon> 	erroneous dprintf expected input.
> 
> This is ok.  Before putting it in could you, if needed, look into the
> printchar thing and then reply?  Thank you.

Like I said, I think it is ok. I will wait a few days so that other people can take a look and see if they find some problems.

Thanks,

Simon