Bug 15806 - Some fields in async MI events get escaped twice
Summary: Some fields in async MI events get escaped twice
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: mi (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 16868 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-07-30 15:42 UTC by asmwarrior
Modified: 2014-09-25 18:14 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Ugly fix (951 bytes, patch)
2014-04-24 15:32 UTC, Simon Marchi
Details | Diff
Less ugly patch (905 bytes, patch)
2014-04-25 19:37 UTC, Simon Marchi
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description asmwarrior 2013-07-30 15:42:21 UTC
Hi, when debug a simple helloworld program under Codeblocks, I have such log (the log is copied from Codeblocks' gdb-debugger-mi plugin's log, so there are some log messages which is the plugin itself)

Active debugger config: GDB/MI:Default
start debugger
Selecting target: 
Debug
Adding file: E:\code\cb\test_code\helloworld\bin\Debug\helloworld.exe

[debug]PATH=.;E:\code\gcc\PCXMinGW463\bin;E:\code\gcc\PCXMinGW463;.;F:\cb_sf_git\trunk\src\lib;E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll;F:\cb_sf_git\trunk\src\devel;E:\code\common_bin;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;D:\Program Files\TortoiseSVN\bin

GDB path: E:\code\gcc\PCXMinGW463\bin\gdb.exe
DEBUGGEE path: E:\code\cb\test_code\helloworld\bin\Debug\helloworld.exe
Command-line: E:\code\gcc\PCXMinGW463\bin\gdb.exe -fullname  -quiet --interpreter=mi -args E:\code\cb\test_code\helloworld\bin\Debug\helloworld.exe
Working dir : E:\code\cb\test_code\helloworld\.
Starting debugger: 

[debug]Executing command: E:\code\gcc\PCXMinGW463\bin\gdb.exe -fullname  -quiet --interpreter=mi -args E:\code\cb\test_code\helloworld\bin\Debug\helloworld.exe

done

[debug]Executor stopped
[debug]Debugger_GDB_MI::CommitBreakpoints
[debug]ActionsMap::Run -> starting action: 0ADA2DE0 id: 1
[debug]BreakpointAddAction::m_initial_cmd = 10000000000
[debug]cmd==>10000000000-break-insert -f E:\code\cb\test_code\helloworld\main.cpp:7
[debug]ActionsMap::Run -> starting action: 060E4E40 id: 2
[debug]cmd==>20000000000-exec-arguments 
[debug]ActionsMap::Run -> starting action: 060E5278 id: 3
[debug]cmd==>30000000000-enable-pretty-printing
[debug]ActionsMap::Run -> starting action: 0A841698 id: 4
[debug]cmd==>40000000000-interpreter-exec console "source E:\\code\\gcc\\PCXMinGW463\\bin\\my.gdb"
[debug]output==>=thread-group-added,id="i1"
[debug]unparsable_output==>~"Reading symbols from E:\\code\\cb\\test_code\\helloworld\\bin\\Debug\\helloworld.exe..."
[debug]notification event recieved!
[debug]unparsable_output==>~"done.\n"
[debug]unparsable_output==>(gdb)
[debug]output==>10000000000^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x004016ee",func="main()",file="E:\\code\\cb\\test_code\\helloworld\\main.cpp",fullname="E:\\code\\cb\\test_code\\helloworld\\main.cpp",line="7",thread-groups=["i1"],times="0",original-location="E:\\code\\cb\\test_code\\helloworld\\main.cpp:7"}
[debug]unparsable_output==>(gdb)
[debug]output==>20000000000^done
[debug]unparsable_output==>(gdb)
[debug]output==>30000000000^done
[debug]unparsable_output==>(gdb)
[debug]output==>=cmd-param-changed,param="filename-display",value="absolute"
[debug]output==>40000000000^done
[debug]unparsable_output==>(gdb)
[debug]BreakpointAddAction::OnCommandResult: 10000000000
[debug]BreakpointAddAction::breakpoint index is 1
[debug]BreakpointAddAction::Finishing1
[debug]notification event recieved!
[debug]BreakpointAddAction::destructor
[debug]ActionsMap::Run -> starting action: 064400A0 id: 5
[debug]RunAction::OnStart -> -exec-run
[debug]cmd==>50000000000-exec-run
[debug]output==>=thread-group-started,id="i1",pid="3080"
[debug]output==>=thread-created,id="1",group-id="i1"
[debug]unparsable_output==>~"[New Thread 3080.0x824]\n"
[debug]output==>50000000000^running
[debug]output==>*running,thread-id="all"
[debug]unparsable_output==>(gdb)
[debug]output==>=library-loaded,id="C:\\WINDOWS\\system32\\ntdll.dll",target-name="C:\\WINDOWS\\system32\\ntdll.dll",host-name="C:\\WINDOWS\\system32\\ntdll.dll",symbols-loaded="0",thread-group="i1"
[debug]output==>=library-loaded,id="C:\\WINDOWS\\system32\\kernel32.dll",target-name="C:\\WINDOWS\\system32\\kernel32.dll",host-name="C:\\WINDOWS\\system32\\kernel32.dll",symbols-loaded="0",thread-group="i1"
[debug]output==>=library-loaded,id="C:\\WINDOWS\\system32\\msvcrt.dll",target-name="C:\\WINDOWS\\system32\\msvcrt.dll",host-name="C:\\WINDOWS\\system32\\msvcrt.dll",symbols-loaded="0",thread-group="i1"
[debug]notification event recieved!

Found child pid: 3080


[debug]notification event recieved!
[debug]RunAction success, the debugger is !stopped!
[debug]RunAction::Output - type: result
class: running
results:

[debug]Executor started
[debug]notification event recieved!
[debug]notification event recieved!
[debug]notification event recieved!
[debug]notification event recieved!
[debug]output==>=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x004016ee",func="main()",file="E:\\\\code\\\\cb\\\\test_code\\\\helloworld\\\\main.cpp",fullname="E:\\\\code\\\\cb\\\\test_code\\\\helloworld\\\\main.cpp",line="7",thread-groups=["i1"],times="1",original-location="E:\\\\code\\\\cb\\\\test_code\\\\helloworld\\\\main.cpp:7"}
[debug]RunAction::destructor
[debug]output==>*stopped,reason="breakpoint-hit",disp="keep",bkptno="1",frame={addr="0x004016ee",func="main",args=[],file="E:\\code\\cb\\test_code\\helloworld\\main.cpp",fullname="E:\\code\\cb\\test_code\\helloworld\\main.cpp",line="7"},thread-id="1",stopped-threads="all"
[debug]unparsable_output==>(gdb)
[debug]notification event recieved!
[debug]notification event recieved!
[debug]Executor stopped

---------------------------------------------------------------------
Here, I simply set a breakpoint, then start running the inferior, than the bp get hit, but I see that there is a message from GDB which I believe is wrong:

file="E:\\\\code\\\\cb\\\\test_code\\\\helloworld\\\\main.cpp"

I think it should be:

file="E:\\code\\cb\\test_code\\helloworld\\main.cpp"

---------------------------------------------------------------------
I'm using the latest GDB cvs 2013-07-30 build myself. (MinGW 4.6.3), the testing code is below:
--------------------------------------
#include <iostream>

using namespace std;

int main()
{
    int a = 0;
    a = 1;
    cout << "Hello world!" << endl;
    return 0;
}
--------------------------------------
Comment 1 asmwarrior 2013-07-31 03:35:56 UTC
When I debug gdb under gdb, I found that in the first step that gdb translate a single backslash to to a double backslash, and in the second step, gdb translate this again, so we finally get a four backslash.

static void
mi_breakpoint_modified (struct breakpoint *b)
{
  struct mi_interp *mi = top_level_interpreter_data ();
  struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
  volatile struct gdb_exception e;

  if (mi_suppress_notification.breakpoint)
    return;

  if (b->number <= 0)
    return;

  target_terminal_ours ();
  fprintf_unfiltered (mi->event_channel,
		      "breakpoint-modified");
  /* We want the output from gdb_breakpoint_query to go to
     mi->event_channel.  One approach would be to just call
     gdb_breakpoint_query, and then use mi_out_put to send the current
     content of mi_outout into mi->event_channel.  However, that will
     break if anything is output to mi_uiout prior to calling the
     breakpoint_created notifications.  So, we use
     ui_out_redirect.  */
  ui_out_redirect (mi_uiout, mi->event_channel);
  TRY_CATCH (e, RETURN_MASK_ERROR)
    gdb_breakpoint_query (mi_uiout, b->number, NULL);
  ui_out_redirect (mi_uiout, NULL);

  gdb_flush (mi->event_channel);
}

First step happens in call gdb_breakpoint_query
Second step happens in gdb_flush function call

which the call stack:
[debug]#0  printchar (c=98, do_fputs=0x60efd2 <fputs_unfiltered>, do_fprintf=0x60c9cb <fprintf_unfiltered>, stream=0x2940a8, quoter=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\utils.c:1507
[debug]#1  0x0060ba96 in fputstrn_unfiltered (str=0x2f7b238 "breakpoint-modified,bkpt={number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"0x00401497\",func=\"main()\",file=\"E:\\\\code\\\\cb\\\\test_code\\\\debug_gdb2011-12-01\\\\main.cpp\",fullname=\"E:\\\\code\\\\cb\\\\tes"..., n=312, quoter=0, stream=0x2940a8) at f:\build_gdb\gdb\gdbgit\gdb\gdb\utils.c:1585
[debug]#2  0x004573c3 in mi_console_raw_packet (data=0x2f11980, buf=0x2f7b238 "breakpoint-modified,bkpt={number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"0x00401497\",func=\"main()\",file=\"E:\\\\code\\\\cb\\\\test_code\\\\debug_gdb2011-12-01\\\\main.cpp\",fullname=\"E:\\\\code\\\\cb\\\\tes"..., length_buf=312) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-console.c:120
[debug]#3  0x0060f4b1 in mem_file_put (file=0x2f119d8, write=0x4572e6 <mi_console_raw_packet>, dest=0x2f11980) at f:\build_gdb\gdb\gdbgit\gdb\gdb\ui-file.c:451
[debug]#4  0x0060ef40 in ui_file_put (file=0x2f119d8, write=0x4572e6 <mi_console_raw_packet>, dest=0x2f11980) at f:\build_gdb\gdb\gdbgit\gdb\gdb\ui-file.c:217
[debug]#5  0x00457442 in mi_console_file_flush (file=0x2f11938) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-console.c:136
[debug]#6  0x0060eef0 in gdb_flush (file=0x2f11938) at f:\build_gdb\gdb\gdbgit\gdb\gdb\ui-file.c:197
[debug]#7  0x0045c986 in mi_breakpoint_modified (b=0x2f93ca8) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-interp.c:707
[debug]#8  0x00562e3c in observer_breakpoint_modified_notification_stub (data=0x45c8a0 <mi_breakpoint_modified>, args_data=0x29df50c) at f:\build_gdb\gdb\gdbgit\debugbuild1\gdb\observer.inc:611
[debug]#9  0x005623d0 in generic_observer_notify (subject=0x2f11fe0, args=0x29df50c) at f:\build_gdb\gdb\gdbgit\gdb\gdb\observer.c:167
[debug]#10 0x00562eba in observer_notify_breakpoint_modified (b=0x2f93ca8) at f:\build_gdb\gdb\gdbgit\debugbuild1\gdb\observer.inc:636
[debug]#11 0x004ad19c in bpstat_stop_status (aspace=0x2f0bce0, bp_addr=4199575, ptid=..., ws=0x29df80c) at f:\build_gdb\gdb\gdbgit\gdb\gdb\breakpoint.c:5293
[debug]#12 0x00517e0f in handle_inferior_event (ecs=0x29df7fc) at f:\build_gdb\gdb\gdbgit\gdb\gdb\infrun.c:4208
[debug]#13 0x005153cc in wait_for_inferior () at f:\build_gdb\gdb\gdbgit\gdb\gdb\infrun.c:2743
[debug]#14 0x005148cb in proceed (addr=2089816591, siggnal=GDB_SIGNAL_0, step=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\infrun.c:2324
[debug]#15 0x0050d9f8 in run_command_1 (args=0x0, from_tty=0, tbreak_at_main=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\infcmd.c:607
[debug]#16 0x0050da28 in run_command (args=0x0, from_tty=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\infcmd.c:617
[debug]#17 0x0044b968 in do_cfunc (c=0x2ee7be8, args=0x0, from_tty=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\cli\cli-decode.c:113
[debug]#18 0x0044e2ce in cmd_func (cmd=0x2ee7be8, args=0x0, from_tty=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\cli\cli-decode.c:1888
[debug]#19 0x0060832a in execute_command (p=0x32cc11b "", from_tty=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\top.c:478
[debug]#20 0x004612b9 in mi_execute_cli_command (cmd=0x7842a4 <__PRETTY_FUNCTION__.23215+297> "run", args_p=0, args=0x0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-main.c:2219
[debug]#21 0x0045df2b in mi_cmd_exec_run (command=0x2f9a8e0 "exec-run", argv=0x2f16668, argc=0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-main.c:406
[debug]#22 0x00461173 in mi_cmd_execute (parse=0x32f8198) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-main.c:2171
[debug]#23 0x004608fd in captured_mi_execute_command (uiout=0x2f11ac0, context=0x32f8198) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-main.c:1922
[debug]#24 0x00460ca7 in mi_execute_command (cmd=0x32cc1d0 "-exec-run", from_tty=1) at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-main.c:2041
[debug]#25 0x0045bf4d in mi_execute_command_wrapper (cmd=0x32cc1d0 "-exec-run") at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-interp.c:311
[debug]#26 0x0045bf60 in mi_execute_command_input_handler (cmd=0x32cc1d0 "-exec-run") at f:\build_gdb\gdb\gdbgit\gdb\gdb\mi\mi-interp.c:319
[debug]#27 0x00532696 in gdb_readline2 (client_data=0x0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\event-top.c:712
[debug]#28 0x00531f00 in stdin_event_handler (error=0, client_data=0x0) at f:\build_gdb\gdb\gdbgit\gdb\gdb\event-top.c:373
[debug]#29 0x005310e2 in handle_file_event (data=...) at f:\build_gdb\gdb\gdbgit\gdb\gdb\event-loop.c:768
[debug](More stack frames follow...)
[debug]>>>>>>cb_gdb:

When you look at the #2, you see that the string is correct, file=\"E:\\\\code, you can see, there is two backslash after the colon. (Note, GDB show char " as \", and show char\ as \\ )

but in printchar() function, there have extra "\" added in the code snippet:
      if (c == '\\' || c == quoter)
	do_fputs ("\\", stream);
      do_fprintf (stream, "%c", c);

So, finally, the two backslash become four backslash, that's why I see the wrong log message.
Anyone can suggest a solution?
Comment 2 asmwarrior 2013-07-31 15:28:59 UTC
Now, I can see the reason:

static void
mi_breakpoint_modified (struct breakpoint *b)
{
  struct mi_interp *mi = top_level_interpreter_data ();
  struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
  volatile struct gdb_exception e;

  if (mi_suppress_notification.breakpoint)
    return;

  if (b->number <= 0)
    return;

  target_terminal_ours ();
  fprintf_unfiltered (mi->event_channel,
		      "breakpoint-modified");
  /* We want the output from gdb_breakpoint_query to go to
     mi->event_channel.  One approach would be to just call
     gdb_breakpoint_query, and then use mi_out_put to send the current
     content of mi_outout into mi->event_channel.  However, that will
     break if anything is output to mi_uiout prior to calling the
     breakpoint_created notifications.  So, we use
     ui_out_redirect.  */
  ui_out_redirect (mi_uiout, mi->event_channel);
  TRY_CATCH (e, RETURN_MASK_ERROR)
    gdb_breakpoint_query (mi_uiout, b->number, NULL);
  ui_out_redirect (mi_uiout, NULL);

  gdb_flush (mi->event_channel);
}

Look at the comment, there is a redirection of the output stream.
In the first step, gdb_breakpoint_query (mi_uiout, b->number, NULL); GDB just output the message for the first time, so one backslash becomes two backslash.
Now, after the ui_out_redirect (mi_uiout, NULL);, in gdb_flush, GDB did again, so two backslash becomes four backslash.

My solution is: we should let the doubling feature only happens in one step.

I just look at other GDB mi messages mechanism, I see that: "gdb_flush (mi->event_channel);" always does a "doubling backslash", so I suggest disable this feature in "gdb_breakpoint_query (mi_uiout, b->number, NULL);".

Any ideas?

Thanks.
Yuanhui Zhang
Comment 3 asmwarrior 2013-12-26 14:53:56 UTC
Some analysis. 
We have many usage of ui_file redirect in GDB source code

For example:

  ui_out_redirect (mi_uiout, mi->event_channel);
  TRY_CATCH (e, RETURN_MASK_ERROR)
    gdb_breakpoint_query (mi_uiout, b->number, NULL);
  ui_out_redirect (mi_uiout, NULL);

  gdb_flush (mi->event_channel);

The contents(buffer) has two chance of running the function: printchar(). When printchar see a \, it will replace it with another \, so we get doubled backslash.

Then, in gdb_flush function, we get another doubled backslash. (the printchar() function also called in gdb_flush), my idea is that: is it possible to avoid calling printchar() in the gdb_flush function?), it looks like the mi_event_channel already have backslash doubled.

Another case is that if a new line character first get chance to becomes \n, then if printchar() called again, it will becomes \\n.
Comment 4 asmwarrior 2013-12-27 02:30:13 UTC
I did some test by disable the double slash generation in the function call of gdb_flush (mi->event_channel); Now, the output has changed from:

=breakpoint-created,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",a
ddr="0x0040162c",func="main()",file="E:\\\\code\\\\cb\\\\test_code\\\\debug_gdb2
011-12-01\\\\main.cpp",fullname="E:\\\\code\\\\cb\\\\test_code\\\\debug_gdb2011-
12-01\\\\main.cpp",line="26",thread-groups=["i1"],times="0",original-location="m
ain()"}

to

=breakpoint-created,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",a
ddr="0x0040162c",func="main()",file="E:\\code\\cb\\test_code\\debug_gdb2011-12-0
1\\main.cpp",fullname="E:\\code\\cb\\test_code\\debug_gdb2011-12-01\\main.cpp",l
ine="26",thread-groups=["i1"],times="0",original-location="main()"}

Looks good, but the side effect is that the other message has changed from:


=library-loaded,id="D:\\mingw-builds\\473\\mingw32\\bin\\libwinpthread-1.dll",ta
rget-name="D:\\mingw-builds\\473\\mingw32\\bin\\libwinpthread-1.dll",host-name="
D:\\mingw-builds\\473\\mingw32\\bin\\libwinpthread-1.dll",symbols-loaded="0",thr
ead-group="i1"

to

=library-loaded,id="D:\mingw-builds\473\mingw32\bin\libwinpthread-1.dll",target-
name="D:\mingw-builds\473\mingw32\bin\libwinpthread-1.dll",host-name="D:\mingw-b
uilds\473\mingw32\bin\libwinpthread-1.dll",symbols-loaded="0",thread-group="i1"

This is bad, because I think the former case in the library-loaded message is correct.

By looking at the source code:

static void
mi_solib_loaded (struct so_list *solib)
{
  struct mi_interp *mi = top_level_interpreter_data ();

  target_terminal_ours ();
  if (gdbarch_has_global_solist (target_gdbarch ()))
    fprintf_unfiltered (mi->event_channel,
			"library-loaded,id=\"%s\",target-name=\"%s\","
			"host-name=\"%s\",symbols-loaded=\"%d\"",
			solib->so_original_name, solib->so_original_name,
			solib->so_name, solib->symbols_loaded);
  else
    fprintf_unfiltered (mi->event_channel,
			"library-loaded,id=\"%s\",target-name=\"%s\","
			"host-name=\"%s\",symbols-loaded=\"%d\","
			"thread-group=\"i%d\"",
			solib->so_original_name, solib->so_original_name,
			solib->so_name, solib->symbols_loaded,
			current_inferior ()->num);

  gdb_flush (mi->event_channel);
}

I see that fprintf_unfltered just output something like "D:\mingw-builds\473..." string to the the mi_event_channel, so if double backslash is disabled in gdb_flush(), we get the result containing "D:\mingw-builds\473...".

So, I don't have an idea to fix this bug.
Comment 5 Simon Marchi 2014-04-24 15:28:44 UTC
*** Bug 16868 has been marked as a duplicate of this bug. ***
Comment 6 Simon Marchi 2014-04-24 15:32:05 UTC
Created attachment 7559 [details]
Ugly fix
Comment 7 Simon Marchi 2014-04-24 15:33:01 UTC
Comment on attachment 7559 [details]
Ugly fix

As a first step, I made this patch which adds a global variable to disable temporarily the escaping. It is a very ugly hack, but I think it gives the behavior we want. Could you test it ?
Comment 8 asmwarrior 2014-04-25 00:52:33 UTC
Hi, Simon, thanks, your patch does not solve my original issue, you patch only fix an issue in the function: 
static void
mi_breakpoint_created (struct breakpoint *b)

But my problem happens in another function:
static void
mi_breakpoint_modified (struct breakpoint *b)

There are many cases we should fix. (Search the usage of ui_out_redirect function call, this issue happens in stream re-direction). As I said before, when a stream get redirected, it has a chance to double the backslash, but finally gdb_flush (mi->event_channel) function call create another new backslash.
Comment 9 asmwarrior 2014-04-25 02:30:31 UTC
Hi, when look at this function:
/* Print the character C on STREAM as part of the contents of a literal
   string whose delimiter is QUOTER.  Note that this routine should only
   be call for printing things which are independent of the language
   of the program being debugged.  */

static void
printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
	   void (*do_fprintf) (struct ui_file *, const char *, ...)
	   ATTRIBUTE_FPTR_PRINTF_2, struct ui_file *stream, int quoter)
{
  c &= 0xFF;			/* Avoid sign bit follies */

  if (c < 0x20 ||		/* Low control chars */
      (c >= 0x7F && c < 0xA0) ||	/* DEL, High controls */
      (sevenbit_strings && c >= 0x80))
    {				/* high order bit set */
      switch (c)
	{
	case '\n':
	  do_fputs ("\\n", stream);
	  break;
	case '\b':
	  do_fputs ("\\b", stream);
	  break;
	case '\t':
	  do_fputs ("\\t", stream);
	  break;
	case '\f':
	  do_fputs ("\\f", stream);
	  break;
	case '\r':
	  do_fputs ("\\r", stream);
	  break;
	case '\033':
	  do_fputs ("\\e", stream);
	  break;
	case '\007':
	  do_fputs ("\\a", stream);
	  break;
	default:
	  do_fprintf (stream, "\\%.3o", (unsigned int) c);
	  break;
	}
    }
  else
    {
      if (c == '\\' || c == quoter)
	do_fputs ("\\", stream);
      do_fprintf (stream, "%c", c);
    }
}

I think we can fix our issue by look at this condition:
1, when   mi->event_channel = mi_console_file_new (raw_stdout, "=", 0); is created, look at the third argument, it is 0, which means the quoter is 0

2, look at my comment 1, I have a call stack shown, where when gdb_flush (mi->event_channel); is called, the deeper printchar will have the last argument quoter = 0.

3, when read the document, I see that "Print the character C on STREAM as part of the contents of a literal string whose delimiter is QUOTER", this means usually the QUOTER is " (for c string) or ' (for c char), but what about quoter == 0?

4, I think if quoter==0, we should not double the backslash.

5, So, we can change the if condition here is like:
      if (c == '\\' ||  (c == quoter && quoter != 0))
	do_fputs ("\\", stream);

6, I initial guess is that if the quoter == 0, we even don't need to escape the \f \r \n (which is the first part of the printchar() function), but I may be wrong.


Yuanhui Zhang(asmwarrior)
Comment 10 asmwarrior 2014-04-25 03:10:02 UTC
(In reply to asmwarrior from comment #9)
> 
> 5, So, we can change the if condition here is like:
>       if (c == '\\' ||  (c == quoter && quoter != 0))
> 	do_fputs ("\\", stream);
> 

Oh, there is a mistake in the above. The correct patch is:
 gdb/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index a8a7cb3..31bdb7e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1539,7 +1539,7 @@ printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
     }
   else
     {
-      if (c == '\\' || c == quoter)
+      if ( (c == '\\' && quoter != 0) || c == quoter)
 	do_fputs ("\\", stream);
       do_fprintf (stream, "%c", c);
     }


But the result is still bad, which lead to some unexpected issue in my comment 4.
Comment 11 asmwarrior 2014-04-25 03:35:16 UTC
@Simon, I think the patch in comment 10 is a good fix. Look at the issue statement in my comment 4, I think the code below
static void
mi_solib_loaded (struct so_list *solib)
{
  struct mi_interp *mi = top_level_interpreter_data ();

  target_terminal_ours ();
  if (gdbarch_has_global_solist (target_gdbarch ()))
    fprintf_unfiltered (mi->event_channel,
			"library-loaded,id=\"%s\",target-name=\"%s\","
			"host-name=\"%s\",symbols-loaded=\"%d\"",
			solib->so_original_name, solib->so_original_name,
			solib->so_name, solib->symbols_loaded);
  else
    fprintf_unfiltered (mi->event_channel,
			"library-loaded,id=\"%s\",target-name=\"%s\","
			"host-name=\"%s\",symbols-loaded=\"%d\","
			"thread-group=\"i%d\"",
			solib->so_original_name, solib->so_original_name,
			solib->so_name, solib->symbols_loaded,
			current_inferior ()->num);

  gdb_flush (mi->event_channel);
}

should fix itself, not the gdb_flush (mi->event_channel). I mean, since mi->event_channel is created with quoter = 0, which means when user put some text in the mi->event_channel, they should do their escape handling themselves.
Once gdb_flush (mi->event_channel) is called, it should not add more backslashes(escape handling).

What do you think?
Comment 12 Simon Marchi 2014-04-25 14:16:38 UTC
Oops, yeah the fix was only for breakpoint-created, sorry about that.

Your comment made me realize that if a library path contains a quote ("), the result will be wrong. Example:

=library-loaded,id="/tmp/hello"you/libpopt.so.0",target-name="/tmp/hello"you/libpopt.so.0",host-name="/tmp/hello"you/libpopt.so.0",symbols-loaded="0",thread-group="i1"

To reproduce this, I created '/tmp/hello"you', copied some library in it and modified LD_LIBRARY_PATH so that my test program use this library instead of the system's version. You can see that the " between hello and you is not escaped, but should be.

I think that no escaping can be done reliably in gdb_flush. There is no way to differentiate a quote that should not be escaped from one that should be escaped. Therefore, I think it should always be done at the moment where the content is created.

Also, using fprintf_unfiltered to output this is wrong, because the fields are never escaped. It would probably be better to use the ui_out_field_* functions everywhere. What do you think ?
Comment 13 asmwarrior 2014-04-25 14:56:30 UTC
(In reply to Simon Marchi from comment #12)
> Oops, yeah the fix was only for breakpoint-created, sorry about that.
Never mind.

> 
> Your comment made me realize that if a library path contains a quote ("),
> the result will be wrong. Example:
> 
> =library-loaded,id="/tmp/hello"you/libpopt.so.0",target-name="/tmp/hello"you/
> libpopt.so.0",host-name="/tmp/hello"you/libpopt.so.0",symbols-loaded="0",
> thread-group="i1"
> 
> To reproduce this, I created '/tmp/hello"you', copied some library in it and
> modified LD_LIBRARY_PATH so that my test program use this library instead of
> the system's version. You can see that the " between hello and you is not
> escaped, but should be.
Yes, look at the function body of mi_solib_loaded (struct so_list *solib). It firstly use fprintf_unfiltered, which directly copy the contents to the stream (such as solib->so_original_name, solib->so_original_name). So, it failed to escape some special chars, such as the " in your file path.

> 
> I think that no escaping can be done reliably in gdb_flush. 
What does this sentence means? You mean gdb_flush should only copy the contents to the UI, it should not do escaping, right?


> There is no way
> to differentiate a quote that should not be escaped from one that should be
> escaped. Therefore, I think it should always be done at the moment where the
> content is created.

Correct, gdb_flush know nothing about whether the buffered stream need to be escaped or not. (unless we write a parser to parse the whole stream, basically, the stream has some format like xxxx="yyyyyy", the parser know whether it is inside a string literal or not, but this way(direction) is surely wrong)

> 
> Also, using fprintf_unfiltered to output this is wrong, because the fields
> are never escaped. It would probably be better to use the ui_out_field_*
> functions everywhere. What do you think ?
Yes, I agree with you, the only way to do escaping correctly is call some ui_out_field_* functions. But this need a lot of code change in GDB.
Comment 14 Simon Marchi 2014-04-25 19:37:40 UTC
Created attachment 7563 [details]
Less ugly patch

Hi Yuanhui,

On top of your fix, I changed mi_solib_loaded to use ui_out_field_* functions, so that the content gets properly escaped. The patch is attached.

So now, both =breakpoint-modified and =library-loaded should be fine at the same time. Could you confirm that? If it is ok, we could identify and correct other functions where fprintf_unfiltered is used directly.

Simon
Comment 15 asmwarrior 2014-04-26 10:03:57 UTC
(In reply to Simon Marchi from comment #14)
> Created attachment 7563 [details]
> Less ugly patch
> 
> Hi Yuanhui,
> 
> On top of your fix, I changed mi_solib_loaded to use ui_out_field_*
> functions, so that the content gets properly escaped. The patch is attached.
> 
> So now, both =breakpoint-modified and =library-loaded should be fine at the
> same time. Could you confirm that? If it is ok, we could identify and
> correct other functions where fprintf_unfiltered is used directly.
> 
> Simon

Hi, Simon, great work, I just tested this patch, and it works fine under Windows. (the issue in comment 4 is solved) I also prefer using ui_out_field_* functions.
Comment 16 Sourceware Commits 2014-06-05 22:02:02 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  6ef284bd18c31645eb3ec4e7691a0f07100d6b4e (commit)
      from  270c9937446ca5273caf7fb102bcdba9ed7cff41 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6ef284bd18c31645eb3ec4e7691a0f07100d6b4e

commit 6ef284bd18c31645eb3ec4e7691a0f07100d6b4e
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Mon Jun 2 17:10:36 2014 -0400

    PR mi/15806: Fix quoting of async events
    
    Original patch:
    https://sourceware.org/ml/gdb-patches/2014-04/msg00552.html
    
    New in v2:
    * In remote.c:escape_buffer, pass '\\' to fputstrn_unfiltered/printchar to
    make sure backslashes are escaped in remote debug output.
    * Updated function documentation for printchar.
    
    See updated ChangeLog below.
    
    --------------------
    
    The quoting in whatever goes in the event_channel of MI is little bit broken.
    
    Link for the lazy:
      https://sourceware.org/bugzilla/show_bug.cgi?id=15806
    
    Here is an example of a =library-loaded event with an ill-named directory,
    /tmp/how"are\you (the problem is present with every directory on Windows since
    it uses backslashes as a path separator). The result will be the following:
    
    =library-loaded,id="/tmp/how"are\\you/libexpat.so.1",...
    
    The " between 'how' and 'are' should be escaped.
    
    Another bad behavior is double escaping in =breakpoint-created, for example:
    
    =breakpoint-created,bkpt={...,fullname="/tmp/how\\"are\\\\you/test.c",...}
    
    The two backslashes before 'how' should be one and the four before 'you' should
    be two.
    
    The reason for this is that when sending something to an MI console, escaping
    can take place at two different moments (the actual escaping work is always
    done in the printchar function):
    
    1. When generating the content, if ui_out_field_* functions are used. Here,
    fields are automatically quoted with " and properly escaped. At least
    mi_field_string does it, not sure about mi_field_fmt, I need to investigate
    further.
    
    2. When gdb_flush is called, to send the data in the buffer of the console to
    the actual output (stdout). At this point, mi_console_raw_packet takes the
    whole string in the buffer, quotes it, and escapes all occurences of the
    quoting character and backslashes. The event_channel does not specify a quoting
    character, so quotes are not escaped here, only backslashes.
    
    The problem with =library-loaded is that it does use fprintf_unfiltered, which
    doesn't do escaping (so, no #1). When gdb_flush is called, backslashes are
    escaped (#2).
    
    The problem with =breakpoint-created is that it first uses ui_out_field_*
    functions to generate its output, so backslashes and quotes are escaped there
    (#1). backslashes are escaped again in #2, leading to an overdose of
    backslashes.
    
    In retrospect, there is no way escaping can be done reliably in
    mi_console_raw_packet for data that is already formatted, such as
    event_channel. At this point, there is no way to differentiate quotes that
    delimit field values from those that should be escaped. In the case of other MI
    consoles, it is ok since mi_console_raw_packet receives one big string that
    should be quoted and escaped as a whole.
    
    So, first part of the fix: for the MI channels that specify no quoting
    character, no escaping at all should be done in mi_console_raw_packet (that's
    the change in printchar, thanks to Yuanhui Zhang for this). For those channels,
    whoever generates the content is responsible for proper quoting and escaping.
    This will fix the =breakpoint-created kind of problem.
    
    Second part of the fix is to make =library-loaded generate content that is
    properly escaped. For this, we use ui_out_field_* functions, instead of one big
    fprintf_unfiltered. =library-unloaded suffered from the same problem so it is
    modified as well. There might be other events that need fixing too, but that's
    all I found with a quick scan. Those that use fprintf_unfiltered but whose sole
    variable data is a %d are not critical, since it won't generate a " or a \.
    
    Finally, a test has been fixed, as it was expecting an erroneous output.
    Otherwise, all other tests that were previously passing still pass (x86-64
    linux).
    
    gdb/ChangeLog:
    
    2014-06-02  Simon Marchi  <simon.marchi@ericsson.com>
    
    	PR mi/15806
    	* utils.c (printchar): Don't escape at all if quoter is NUL.
    	Update function documentation to clarify effect of parameter
    	QUOTER.
    	* remote.c (escape_buffer): Pass '\\' as the quoter to
    	fputstrn_unfiltered.
    	* mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
    	generate the output.
    	(mi_solib_unloaded): Same.
    
    gdb/testsuite/ChangeLog:
    
    2014-06-02  Simon Marchi  <simon.marchi@ericsson.com>
    
    	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
    	erroneous dprintf expected input.

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

Summary of changes:
 gdb/ChangeLog                                  |   12 +++++
 gdb/mi/mi-interp.c                             |   57 +++++++++++++-----------
 gdb/remote.c                                   |    2 +-
 gdb/testsuite/ChangeLog                        |    5 ++
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    2 +-
 gdb/utils.c                                    |   10 +++-
 6 files changed, 58 insertions(+), 30 deletions(-)
Comment 17 Simon Marchi 2014-09-25 18:14:31 UTC
Fixed in 6ef284bd18c31645eb3ec4e7691a0f07100d6b4e.