Re: [patch] Do not open Python scripts twice #2 [Re: [RFC] Crash sourcing Python script on Windows]

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 23 Jan 2012 19:38:34 +0100, Pedro Alves wrote:
> > +# MS-Windows platform may have Python compiled with different libc
> > +# having the FILE structure layout incompatible between libraries.
> 
> It is not just about the layout.  The layout of both libc's could be the
> same, but you'd still (maybe) crash.  It's more about the private global
> state maintained by each of the libc's.  E.g., file descriptor N of
> gdb's libc has no meaning the python's libc -- each libc has its own
> file descriptor table.

I have updated the text.


> This is undefined behavior in the supposedly crashing case.  Hopefully this
> doesn't happen to succeed by running some other random program.  :-)

Yes, my fear is that if for example the only problem is fd assignments it may
try to read from a closed descriptor possibly behaving as empty file and it
would falsely pass.  Better test would be it did really read some test Python
code but a minimal such testcase looks pretty complicated to me for configure.
But when Joel was talking about "crash" this simple test may be enough.


> The patch looks good to me.

These are the reasons why I would prefer a real MS-Windows test of it.


Thanks,
Jan


gdb/
2012-01-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Do not open script filenames twice.
	* cli/cli-cmds.c (source_script_from_stream): Pass to
	source_python_script also STREAM.
	* config.in: Regenerate.
	* configure: Regenerate.
	* configure.ac (HAVE_PYTHON_FILEP_COMPAT): New test.
	* python/py-auto-load.c (source_section_scripts): Pass to
	source_python_script_for_objfile also STREAM.
	(auto_load_objfile_script): Pass to source_python_script_for_objfile
	also INPUT.
	* python/python-internal.h (source_python_script_for_objfile): New
	parameter file, rename parameter file to filename.
	* python/python.c (python_run_simple_file): Call PyRun_SimpleFile
	instead if HAVE_PYTHON_FILEP_COMPAT.
	(source_python_script, source_python_script_for_objfile)
	(source_python_script): New parameter file, rename parameter file to
	filename.  Pass FILENAME to python_run_simple_file.
	* python/python.h (source_python_script): New parameter file, rename
	parameter file to filename.

--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -540,9 +540,7 @@ source_script_from_stream (FILE *stream, const char *file)
 
       TRY_CATCH (e, RETURN_MASK_ERROR)
 	{
-          /* The python support reopens the file using python functions,
-             so there's no point in passing STREAM here.  */
-	  source_python_script (file);
+	  source_python_script (stream, file);
 	}
       if (e.reason < 0)
 	{
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1566,6 +1566,47 @@ aix*)
   ;;
 esac
 
+# Python library may be built with its own copy of libc - typically on
+# MS-Windows platform.  Passing FILE * from GDB to libpython is then
+# incompatible as FILE is related to the specific libc internal state.
+if test "${have_libpython}" != no; then
+  AC_CACHE_CHECK([whether FILE * is compatible with Python],
+		 [gdb_cv_python_filep_compat], [
+    old_CFLAGS="$CFLAGS"
+    CFLAGS="$CFLAGS $PYTHON_CFLAGS"
+    old_CPPFLAGS="$CPPFLAGS"
+    CPPFLAGS="$CPPFLAGS $PYTHON_CPPFLAGS"
+    old_LIBS="$LIBS"
+    LIBS="$LIBS $PYTHON_LIBS"
+    : >conftest.empty
+    gdb_cv_python_filep_compat=no
+    AC_RUN_IFELSE(
+      AC_LANG_PROGRAM(
+	[#include "]${have_libpython}[/Python.h"
+	 #include <stdio.h>],
+	[const char *filename = "conftest.empty";
+	 FILE *f = fopen (filename, "r");
+	 if (f == NULL)
+	  return 1;
+	 Py_Initialize ();
+	 PyRun_File (f, filename, Py_file_input, PyDict_New (), PyDict_New ());
+	 if (PyErr_Occurred ())
+	   {
+	     PyErr_Print ();
+	     return 1;
+	   }
+	 return 0;]),
+      [gdb_cv_python_filep_compat=yes], [], [true])
+    CFLAGS="$old_CFLAGS"
+    CPPFLAGS="$old_CPPFLAGS"
+    LIBS="$old_LIBS"
+  ])
+  if test x$gdb_cv_python_filep_compat = xyes; then
+    AC_DEFINE(HAVE_PYTHON_FILEP_COMPAT, 1,
+	      [Define if libpython can be passed FILE * without crashing.])
+  fi
+fi
+
 AC_MSG_CHECKING(for the dynamic export flag)
 dynamic_list=false
 if test "${gdb_native}" = yes; then
--- a/gdb/python/py-auto-load.c
+++ b/gdb/python/py-auto-load.c
@@ -312,7 +312,7 @@ Use `info auto-load-scripts [REGEXP]' to list them."),
 	{
 	  /* If this file is not currently loaded, load it.  */
 	  if (! in_hash_table)
-	    source_python_script_for_objfile (objfile, full_path);
+	    source_python_script_for_objfile (objfile, stream, full_path);
 	  fclose (stream);
 	  xfree (full_path);
 	}
@@ -431,7 +431,7 @@ auto_load_objfile_script (struct objfile *objfile, const char *suffix)
 	 It's highly unlikely that we'd ever load it twice,
 	 and these scripts are required to be idempotent under multiple
 	 loads anyway.  */
-      source_python_script_for_objfile (objfile, debugfile);
+      source_python_script_for_objfile (objfile, input, debugfile);
       fclose (input);
     }
 
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -289,8 +289,8 @@ extern const struct language_defn *python_language;
 
 void gdbpy_print_stack (void);
 
-void source_python_script_for_objfile (struct objfile *objfile,
-				       const char *file);
+void source_python_script_for_objfile (struct objfile *objfile, FILE *file,
+				       const char *filename);
 
 PyObject *python_string_to_unicode (PyObject *obj);
 char *unicode_to_target_string (PyObject *unicode_str);
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -151,8 +151,8 @@ ensure_python_env (struct gdbarch *gdbarch,
   return make_cleanup (restore_python_env, env);
 }
 
-/* A wrapper around PyRun_SimpleFile.  FILENAME is the name of
-   the Python script to run.
+/* A wrapper around PyRun_SimpleFile.  FILE is the Python script to run
+   named FILENAME.
 
    One of the parameters of PyRun_SimpleFile is a FILE *.
    The problem is that type FILE is extremely system and compiler
@@ -177,8 +177,14 @@ ensure_python_env (struct gdbarch *gdbarch,
    with the Python library.  */
 
 static void
-python_run_simple_file (const char *filename)
+python_run_simple_file (FILE *file, const char *filename)
 {
+#ifdef HAVE_PYTHON_FILEP_COMPAT
+
+  PyRun_SimpleFile (file, filename);
+
+#else /* !HAVE_PYTHON_FILEP_COMPAT */
+
   char *full_path;
   PyObject *python_file;
   struct cleanup *cleanup;
@@ -198,6 +204,8 @@ python_run_simple_file (const char *filename)
   make_cleanup_py_decref (python_file);
   PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
   do_cleanups (cleanup);
+
+#endif /* !HAVE_PYTHON_FILEP_COMPAT */
 }
 
 /* Given a command_line, return a command string suitable for passing
@@ -620,17 +628,17 @@ gdbpy_parse_and_eval (PyObject *self, PyObject *args)
 }
 
 /* Read a file as Python code.
-   FILE is the name of the file.
+   FILE is the file to run.  FILENAME is name of the file FILE.
    This does not throw any errors.  If an exception occurs python will print
    the traceback and clear the error indicator.  */
 
 void
-source_python_script (const char *file)
+source_python_script (FILE *file, const char *filename)
 {
   struct cleanup *cleanup;
 
   cleanup = ensure_python_env (get_current_arch (), current_language);
-  python_run_simple_file (file);
+  python_run_simple_file (file, filename);
   do_cleanups (cleanup);
 }
 
@@ -991,19 +999,20 @@ gdbpy_progspaces (PyObject *unused1, PyObject *unused2)
    source_python_script_for_objfile; it is NULL at other times.  */
 static struct objfile *gdbpy_current_objfile;
 
-/* Set the current objfile to OBJFILE and then read FILE as Python code.
-   This does not throw any errors.  If an exception occurs python will print
-   the traceback and clear the error indicator.  */
+/* Set the current objfile to OBJFILE and then read FILE named FILENAME
+   as Python code.  This does not throw any errors.  If an exception
+   occurs python will print the traceback and clear the error indicator.  */
 
 void
-source_python_script_for_objfile (struct objfile *objfile, const char *file)
+source_python_script_for_objfile (struct objfile *objfile, FILE *file,
+                                  const char *filename)
 {
   struct cleanup *cleanups;
 
   cleanups = ensure_python_env (get_objfile_arch (objfile), current_language);
   gdbpy_current_objfile = objfile;
 
-  python_run_simple_file (file);
+  python_run_simple_file (file, filename);
 
   do_cleanups (cleanups);
   gdbpy_current_objfile = NULL;
@@ -1079,7 +1088,7 @@ eval_python_from_control_command (struct command_line *cmd)
 }
 
 void
-source_python_script (const char *file)
+source_python_script (FILE *file, const char *filename)
 {
   throw_error (UNSUPPORTED_ERROR,
 	       _("Python scripting is not supported in this copy of GDB."));
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -30,7 +30,7 @@ extern void finish_python_initialization (void);
 
 void eval_python_from_control_command (struct command_line *);
 
-void source_python_script (const char *file);
+void source_python_script (FILE *file, const char *filename);
 
 int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
 			      int embedded_offset, CORE_ADDR address,