diff mbox

[v3,9/9] compile: compile printf: gdbserver support

Message ID 20150411194437.29128.58569.stgit@host1.jankratochvil.net
State New, archived
Headers

Commit Message

Jan Kratochvil April 11, 2015, 7:44 p.m. UTC 
  Hi,

former patch injects plain:
	printf (...);
This patch injects gdbserver-compatible:
	f = open_memstream (&s, ...);
	fprintf (f, ...);
	fclose (f);
	return s;

Jan


gdb/
2015-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* compile/compile-c-support.c (add_code_header, add_code_footer)
	(c_compute_program): Use open_memstream, fprintf and fclose.
	* compile/compile-object-load.c (compile_object_load): Set expected
	char * for COMPILE_I_PRINTF_SCOPE.
	* compile/compile-object-run.c: Include gdbcore.h.
	(free_inferior_memory): New function.
	(compile_object_run): Support COMPILE_I_PRINTF_SCOPE's return value.

gdb/doc/ChangeLog
2015-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Compiling and Injecting Code): Mention for compile
	printf open_memstream and fprintf.

gdb/testsuite/ChangeLog
2015-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.compile/compile-print.exp (compile printf "0x%x\n", varint)
	(compile printf "0x%x\n"): Remove !is_remote conditional.

Comments

Jan Kratochvil April 26, 2015, 9:33 a.m. UTC | #1 
On Sat, 11 Apr 2015 21:44:37 +0200, Jan Kratochvil wrote:
> former patch injects plain:
> 	printf (...);
> This patch injects gdbserver-compatible:
> 	f = open_memstream (&s, ...);
> 	fprintf (f, ...);
> 	fclose (f);
> 	return s;

I have realized this print+printf patchset introduces calling inferior
implicit malloc() + explicit free() (by free_inferior_memory) which the
original 'compile code' series avoided (using gdbarch_infcall_mmap() instead).
The goal was not to crash the inferior futher with print commands when
analyzing corrupted inferior memory lists.

I somehow expected that printf()/fprintf() are so heavyweight they will call
malloc() on their own so this mmap goal is no longer achievable for printf.
But I have found now glibc in most real world cases uses just alloca().

The problem is even calling fmemopen() instead of open_memstream() still
implicitly calls malloc() - for fmemopen_cookie_t and for FILE.

The only idea I have is to redirect by a breakpoint glibc's implicit calls to
malloc() into GDB's allocator by inferior mmap.  But that seems a bit ugly.

So currently keeping it as a known bug.


Jan
Pedro Alves April 29, 2015, 3:52 p.m. UTC | #2 
On 04/11/2015 08:44 PM, Jan Kratochvil wrote:
> Hi,
> 
> former patch injects plain:
> 	printf (...);
> This patch injects gdbserver-compatible:
> 	f = open_memstream (&s, ...);
> 	fprintf (f, ...);
> 	fclose (f);
> 	return s;

(A more expanded explanation would have helped here.  The first time I
skimmed this, I didn't really understand what this meant.)

So the issue here is that calling "printf" in the inferior ends up
with output sent to the inferior's stdout.  If gdbserver is running on a
separate terminal (or machine), then the output of "compile printf" without
this patch goes to the inferior's terminal, unlike using "(gdb) printf ...".

That's not just an issue for gdbserver, actually.  Even with the native target,
using "compile printf" without this patch against an inferior that gdb attached
to, with "attach PID" (a process that was already running on a separate
terminal), or if you use the "set inferior-tty" option, you get the exact
same problem.

> @@ -233,10 +241,15 @@ add_code_footer (enum compile_i_scope_types type, struct ui_file *buf)
>  {
>    switch (type)
>      {
> +    case COMPILE_I_PRINTF_SCOPE:
> +      fputs_unfiltered ("\tif (fclose (__gdb_outf) != 0)\n"
> +			"\t\treturn NULL;\n"
> +			"\treturn __gdb_retval;\n",
> +			buf);
> +      // FALLTHRU

Please use /* */ comments, here and elsewhere.

(replying to the other mail)

Thanks,
Pedro Alves
Pedro Alves April 29, 2015, 3:54 p.m. UTC | #3 
On 04/26/2015 10:33 AM, Jan Kratochvil wrote:
> On Sat, 11 Apr 2015 21:44:37 +0200, Jan Kratochvil wrote:
>> former patch injects plain:
>> 	printf (...);
>> This patch injects gdbserver-compatible:
>> 	f = open_memstream (&s, ...);
>> 	fprintf (f, ...);
>> 	fclose (f);
>> 	return s;
> 
> I have realized this print+printf patchset introduces calling inferior
> implicit malloc() + explicit free() (by free_inferior_memory) which the
> original 'compile code' series avoided (using gdbarch_infcall_mmap() instead).
> The goal was not to crash the inferior futher with print commands when
> analyzing corrupted inferior memory lists.

Right.  The "compile code" infrastructure should restrict itself
to async-signal-safe functions for its internal mechanisms for that reason.
Of course, if the expression the user injects runs non-async-signal-safe
at the wrong time, the user gets what she asked for.

> 
> I somehow expected that printf()/fprintf() are so heavyweight they will call
> malloc() on their own so this mmap goal is no longer achievable for printf.
> But I have found now glibc in most real world cases uses just alloca().
> 
> The problem is even calling fmemopen() instead of open_memstream() still
> implicitly calls malloc() - for fmemopen_cookie_t and for FILE.
> 
> The only idea I have is to redirect by a breakpoint glibc's implicit calls to
> malloc() into GDB's allocator by inferior mmap.  But that seems a bit ugly.

Using mmap along with snprintf would be safer, but given that snprintf is
not async-signal-safe in general either, it's fine with me to leave this
as you have it.

I think the manual should say that the command internally may call
functions that are not async-signal-safe though.

> So currently keeping it as a known bug.

Otherwise looks good to me.

Thanks,
Pedro Alves
Jan Kratochvil May 3, 2015, 2:06 p.m. UTC | #4 
On Wed, 29 Apr 2015 17:54:11 +0200, Pedro Alves wrote:
> On 04/26/2015 10:33 AM, Jan Kratochvil wrote:
> > The only idea I have is to redirect by a breakpoint glibc's implicit calls to
> > malloc() into GDB's allocator by inferior mmap.  But that seems a bit ugly.
> 
> Using mmap along with snprintf would be safer, but given that snprintf is
> not async-signal-safe in general either, it's fine with me to leave this
> as you have it.

OK, snprintf into mmap()ped buffer looks easier.  While snprintf is not
async-signal-safe in general IMO it is async-signal-safe for most format
strings given in almost always uses alloca() instead of malloc().
Or do you realize other problems it may have?

Still I am sure fine to check it in as is if it is approved this way.


> I think the manual should say that the command internally may call
> functions that are not async-signal-safe though.

Done.


Thanks,
Jan
Pedro Alves May 6, 2015, 10:22 a.m. UTC | #5 
On 05/03/2015 03:06 PM, Jan Kratochvil wrote:
> On Wed, 29 Apr 2015 17:54:11 +0200, Pedro Alves wrote:
>> On 04/26/2015 10:33 AM, Jan Kratochvil wrote:
>>> The only idea I have is to redirect by a breakpoint glibc's implicit calls to
>>> malloc() into GDB's allocator by inferior mmap.  But that seems a bit ugly.
>>
>> Using mmap along with snprintf would be safer, but given that snprintf is
>> not async-signal-safe in general either, it's fine with me to leave this
>> as you have it.
> 
> OK, snprintf into mmap()ped buffer looks easier.  While snprintf is not
> async-signal-safe in general IMO it is async-signal-safe for most format
> strings given in almost always uses alloca() instead of malloc().

Right.

> Or do you realize other problems it may have?

Nope.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index 92c41b6..3b9bb52 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -208,13 +208,21 @@  add_code_header (enum compile_i_scope_types type, struct ui_file *buf)
       break;
     case COMPILE_I_PRINTF_SCOPE:
       fputs_unfiltered ("#include <stdio.h>\n"
-			"void "
+			"char *"
 			GCC_FE_WRAPPER_FUNCTION
 			" (struct "
 			COMPILE_I_SIMPLE_REGISTER_STRUCT_TAG
 			" *"
 			COMPILE_I_SIMPLE_REGISTER_ARG_NAME
-			") {\n",
+			") {\n"
+			"\tchar *__gdb_retval;\n"
+			"\tsize_t __gdb_retval_size;\n"
+			"\tFILE *__gdb_outf;\n"
+			"\n"
+			"\t__gdb_outf = open_memstream (&__gdb_retval, "
+						       "&__gdb_retval_size);\n"
+			"\tif (__gdb_outf == NULL)\n"
+			"\t\treturn NULL;\n",
 			buf);
       break;
     case COMPILE_I_RAW_SCOPE:
@@ -233,10 +241,15 @@  add_code_footer (enum compile_i_scope_types type, struct ui_file *buf)
 {
   switch (type)
     {
+    case COMPILE_I_PRINTF_SCOPE:
+      fputs_unfiltered ("\tif (fclose (__gdb_outf) != 0)\n"
+			"\t\treturn NULL;\n"
+			"\treturn __gdb_retval;\n",
+			buf);
+      // FALLTHRU
     case COMPILE_I_SIMPLE_SCOPE:
     case COMPILE_I_PRINT_ADDRESS_SCOPE:
     case COMPILE_I_PRINT_VALUE_SCOPE:
-    case COMPILE_I_PRINTF_SCOPE:
       fputs_unfiltered ("}\n", buf);
       break;
     case COMPILE_I_RAW_SCOPE:
@@ -432,7 +445,7 @@  c_compute_program (struct compile_instance *inst,
       break;
     case COMPILE_I_PRINTF_SCOPE:
       fprintf_unfiltered (buf,
-"printf (%s);\n"
+"fprintf (__gdb_outf, %s);\n"
 			  , input);
       break;
     default:
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index bc74590..d7c13c9 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -651,7 +651,8 @@  compile_object_load (const char *object_file, const char *source_file,
       break;
     case COMPILE_I_PRINTF_SCOPE:
       expect_parameters = 1;
-      expect_return_type = builtin_type (target_gdbarch ())->builtin_void;
+      expect_return_type = lookup_pointer_type
+			       (builtin_type (target_gdbarch ())->builtin_char);
       break;
     default:
       internal_error (__FILE__, __LINE__, _("invalid scope %d"), scope);
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 216cbf9..ed56410 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -27,6 +27,7 @@ 
 #include "block.h"
 #include "valprint.h"
 #include "compile.h"
+#include "gdbcore.h"

 /* Helper for do_module_cleanup.  */

@@ -99,6 +100,18 @@  do_module_cleanup (void *arg)
   xfree (data);
 }

+static void
+free_inferior_memory (CORE_ADDR addr)
+{
+  struct objfile *objf;
+  struct value *func_val = find_function_in_inferior ("free", &objf);
+  struct gdbarch *gdbarch = get_objfile_arch (objf);
+  struct type *addr_type = builtin_type (gdbarch)->builtin_data_ptr;
+  struct value *addr_val = value_from_pointer (addr_type, addr);
+
+  call_function_by_hand (func_val, 1, &addr_val);
+}
+
 /* Perform inferior call of MODULE.  This function may throw an error.
    This function may leave files referenced by MODULE on disk until
    the inferior call dummy frame is discarded.  This function may throw errors.
@@ -117,6 +130,7 @@  compile_object_run (struct compile_module *module)
   struct symbol *func_sym = module->func_sym;
   CORE_ADDR regs_addr = module->regs_addr;
   struct objfile *objfile = module->objfile;
+  enum compile_i_scope_types scope = module->scope;

   data = xmalloc (sizeof (*data) + strlen (objfile_name_s));
   data->executedp = &executed;
@@ -136,7 +150,7 @@  compile_object_run (struct compile_module *module)
       struct type *func_type = SYMBOL_TYPE (func_sym);
       htab_t copied_types;
       int current_arg = 0;
-      struct value **vargs;
+      struct value **vargs, *func_return_value;

       /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
       copied_types = create_copied_types_hash (objfile);
@@ -163,8 +177,42 @@  compile_object_run (struct compile_module *module)
 	  ++current_arg;
 	}
       gdb_assert (current_arg == TYPE_NFIELDS (func_type));
-      call_function_by_hand_dummy (func_val, TYPE_NFIELDS (func_type), vargs,
-				   do_module_cleanup, data);
+      func_return_value = call_function_by_hand_dummy (func_val,
+						       TYPE_NFIELDS (func_type),
+						       vargs,
+						       do_module_cleanup, data);
+
+      // DATA can be already freed now.
+      data = NULL;
+
+      if (scope == COMPILE_I_PRINTF_SCOPE)
+	{
+	  struct value_print_options opts;
+	  gdb_byte *buffer = NULL;
+	  struct cleanup *old_chain;
+	  int errcode, bytes_read;
+	  struct type *retval_type = value_type (func_return_value);
+	  struct type *char_type;
+
+	  gdb_assert (TYPE_CODE (retval_type) == TYPE_CODE_PTR);
+	  char_type = TYPE_TARGET_TYPE (retval_type);
+	  gdb_assert (TYPE_CODE (char_type) == TYPE_CODE_INT);
+
+	  get_user_print_options (&opts);
+	  errcode = read_string (value_as_address (func_return_value), -1,
+				 TYPE_LENGTH (char_type), opts.print_max,
+				 gdbarch_byte_order (target_gdbarch ()),
+				 &buffer, &bytes_read);
+	  old_chain = make_cleanup (xfree, buffer);
+	  if (errcode != 0)
+	    memory_error (errcode, value_as_address (func_return_value));
+
+	  while (bytes_read-- > 0)
+	    putchar_filtered (*buffer++);
+	  do_cleanups (old_chain);
+
+	  free_inferior_memory (value_as_address (func_return_value));
+	}
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
diff --git a/gdb/testsuite/gdb.compile/compile-print.exp b/gdb/testsuite/gdb.compile/compile-print.exp
index 14fd489..43b9f24 100644
--- a/gdb/testsuite/gdb.compile/compile-print.exp
+++ b/gdb/testsuite/gdb.compile/compile-print.exp
@@ -57,7 +57,5 @@  gdb_test {print $} " = 256"

 gdb_test "compile print varobject" { = {field = 1}}

-if ![is_remote target] {
-    gdb_test {compile printf "0x%x\n", varint} "\r\n0xa"
-    gdb_test {compile printf "0x%x\n"} "\r\nCompilation failed\\."
-}
+gdb_test {compile printf "0x%x\n", varint} "\r\n0xa"
+gdb_test {compile printf "0x%x\n"} "\r\nCompilation failed\\."
---
 gdb/doc/gdb.texinfo |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index 92c41b6..3b9bb52 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -208,13 +208,21 @@  add_code_header (enum compile_i_scope_types type, struct ui_file *buf)
       break;
     case COMPILE_I_PRINTF_SCOPE:
       fputs_unfiltered ("#include <stdio.h>\n"
-			"void "
+			"char *"
 			GCC_FE_WRAPPER_FUNCTION
 			" (struct "
 			COMPILE_I_SIMPLE_REGISTER_STRUCT_TAG
 			" *"
 			COMPILE_I_SIMPLE_REGISTER_ARG_NAME
-			") {\n",
+			") {\n"
+			"\tchar *__gdb_retval;\n"
+			"\tsize_t __gdb_retval_size;\n"
+			"\tFILE *__gdb_outf;\n"
+			"\n"
+			"\t__gdb_outf = open_memstream (&__gdb_retval, "
+						       "&__gdb_retval_size);\n"
+			"\tif (__gdb_outf == NULL)\n"
+			"\t\treturn NULL;\n",
 			buf);
       break;
     case COMPILE_I_RAW_SCOPE:
@@ -233,10 +241,15 @@  add_code_footer (enum compile_i_scope_types type, struct ui_file *buf)
 {
   switch (type)
     {
+    case COMPILE_I_PRINTF_SCOPE:
+      fputs_unfiltered ("\tif (fclose (__gdb_outf) != 0)\n"
+			"\t\treturn NULL;\n"
+			"\treturn __gdb_retval;\n",
+			buf);
+      // FALLTHRU
     case COMPILE_I_SIMPLE_SCOPE:
     case COMPILE_I_PRINT_ADDRESS_SCOPE:
     case COMPILE_I_PRINT_VALUE_SCOPE:
-    case COMPILE_I_PRINTF_SCOPE:
       fputs_unfiltered ("}\n", buf);
       break;
     case COMPILE_I_RAW_SCOPE:
@@ -432,7 +445,7 @@  c_compute_program (struct compile_instance *inst,
       break;
     case COMPILE_I_PRINTF_SCOPE:
       fprintf_unfiltered (buf,
-"printf (%s);\n"
+"fprintf (__gdb_outf, %s);\n"
 			  , input);
       break;
     default:
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index bc74590..d7c13c9 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -651,7 +651,8 @@  compile_object_load (const char *object_file, const char *source_file,
       break;
     case COMPILE_I_PRINTF_SCOPE:
       expect_parameters = 1;
-      expect_return_type = builtin_type (target_gdbarch ())->builtin_void;
+      expect_return_type = lookup_pointer_type
+			       (builtin_type (target_gdbarch ())->builtin_char);
       break;
     default:
       internal_error (__FILE__, __LINE__, _("invalid scope %d"), scope);
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 216cbf9..3606d42 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -27,6 +27,7 @@ 
 #include "block.h"
 #include "valprint.h"
 #include "compile.h"
+#include "gdbcore.h"
 
 /* Helper for do_module_cleanup.  */
 
@@ -99,6 +100,20 @@  do_module_cleanup (void *arg)
   xfree (data);
 }
 
+/* Call inferior function "free" for ADDR.  */
+
+static void
+free_inferior_memory (CORE_ADDR addr)
+{
+  struct objfile *objf;
+  struct value *func_val = find_function_in_inferior ("free", &objf);
+  struct gdbarch *gdbarch = get_objfile_arch (objf);
+  struct type *addr_type = builtin_type (gdbarch)->builtin_data_ptr;
+  struct value *addr_val = value_from_pointer (addr_type, addr);
+
+  call_function_by_hand (func_val, 1, &addr_val);
+}
+
 /* Perform inferior call of MODULE.  This function may throw an error.
    This function may leave files referenced by MODULE on disk until
    the inferior call dummy frame is discarded.  This function may throw errors.
@@ -117,6 +132,7 @@  compile_object_run (struct compile_module *module)
   struct symbol *func_sym = module->func_sym;
   CORE_ADDR regs_addr = module->regs_addr;
   struct objfile *objfile = module->objfile;
+  enum compile_i_scope_types scope = module->scope;
 
   data = xmalloc (sizeof (*data) + strlen (objfile_name_s));
   data->executedp = &executed;
@@ -136,7 +152,7 @@  compile_object_run (struct compile_module *module)
       struct type *func_type = SYMBOL_TYPE (func_sym);
       htab_t copied_types;
       int current_arg = 0;
-      struct value **vargs;
+      struct value **vargs, *func_return_value;
 
       /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
       copied_types = create_copied_types_hash (objfile);
@@ -163,8 +179,42 @@  compile_object_run (struct compile_module *module)
 	  ++current_arg;
 	}
       gdb_assert (current_arg == TYPE_NFIELDS (func_type));
-      call_function_by_hand_dummy (func_val, TYPE_NFIELDS (func_type), vargs,
-				   do_module_cleanup, data);
+      func_return_value = call_function_by_hand_dummy (func_val,
+						       TYPE_NFIELDS (func_type),
+						       vargs,
+						       do_module_cleanup, data);
+
+      // DATA can be already freed now.
+      data = NULL;
+
+      if (scope == COMPILE_I_PRINTF_SCOPE)
+	{
+	  struct value_print_options opts;
+	  gdb_byte *buffer = NULL;
+	  struct cleanup *old_chain;
+	  int errcode, bytes_read;
+	  struct type *retval_type = value_type (func_return_value);
+	  struct type *char_type;
+
+	  gdb_assert (TYPE_CODE (retval_type) == TYPE_CODE_PTR);
+	  char_type = TYPE_TARGET_TYPE (retval_type);
+	  gdb_assert (TYPE_CODE (char_type) == TYPE_CODE_INT);
+
+	  get_user_print_options (&opts);
+	  errcode = read_string (value_as_address (func_return_value), -1,
+				 TYPE_LENGTH (char_type), opts.print_max,
+				 gdbarch_byte_order (target_gdbarch ()),
+				 &buffer, &bytes_read);
+	  old_chain = make_cleanup (xfree, buffer);
+	  if (errcode != 0)
+	    memory_error (errcode, value_as_address (func_return_value));
+
+	  while (bytes_read-- > 0)
+	    putchar_filtered (*buffer++);
+	  do_cleanups (old_chain);
+
+	  free_inferior_memory (value_as_address (func_return_value));
+	}
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6688674..fd7ff05 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -17239,7 +17239,9 @@  multiple-line editor.
 @item compile printf @var{template}, @var{expressions}@dots{}
 Compile and execute @code{printf} function call with the compiler
 language found as the current language in @value{GDBN}
-(@pxref{Languages}).
+(@pxref{Languages}).  The value is printed by @value{GDBN} and not the
+inferior, inferior does not execute specifically the function
+@code{printf}.
 
 @item compile printf
 @cindex reprint the last value
diff --git a/gdb/testsuite/gdb.compile/compile-print.exp b/gdb/testsuite/gdb.compile/compile-print.exp
index 14fd489..43b9f24 100644
--- a/gdb/testsuite/gdb.compile/compile-print.exp
+++ b/gdb/testsuite/gdb.compile/compile-print.exp
@@ -57,7 +57,5 @@  gdb_test {print $} " = 256"
 
 gdb_test "compile print varobject" { = {field = 1}}
 
-if ![is_remote target] {
-    gdb_test {compile printf "0x%x\n", varint} "\r\n0xa"
-    gdb_test {compile printf "0x%x\n"} "\r\nCompilation failed\\."
-}
+gdb_test {compile printf "0x%x\n", varint} "\r\n0xa"
+gdb_test {compile printf "0x%x\n"} "\r\nCompilation failed\\."