[RFC] Crash sourcing Python script on Windows

[RFC] Crash sourcing Python script on Windows

[Joel Brobecker]

Hello,

At long last, I forced myself to look into this. I had noticed
that "source script.py" did not work on Windows, and resulted
in the debugger crashing.  It turns out that the problem came
from the fact that I was using a binaries from python.org where
the definition of type FILE difers from the definition our
compiler uses.  The crash occurs when trying to call PyRun_SimpleFile
with a FILE created by GDB.

The approach we took is to create a new FILE descriptor using
Python routines, thus making sure that the FILE is compatible
with PyRun_SimpleFile.  There were two such calls, so we wrote
a wrapper to that function that only takes a filename, and creates
the FILE on the fly.  This avoids code duplication.

We typically build Python ourselves on all platforms, except
Windows, where the build setup is a lot different. That's why
we have only ever seen this problem there.  Still, this is
arguably a problem that could occur on other systems, even if
I somehow doubt it (this is a guess!). As a result, I decided
to apply the workaround unconditionally on all platforms.
I don't see this as a big performance impact in practice.

The internet if full of references to people hitting the same
problem.  So I think we should make an attempt at preventing
this from happening. As said above, building Python on Windows
is hard, so we shouldn't expect typical developers to do themselves.

The question, hence the RFC, is: can anyone see a better way?
I thought about reading the entire script from stream to a string,
and then feeding that to PyRun_SimpleString.  That would work too,
I think.   But reading a file into a string means that we'd have
to have the entire file in memory, whereas the python interpreter
might have a smarter strategy.

Anyways, open to other options...

gdb/ChangeLog:

        * python/python.c (python_run_simple_file): New function.
        (source_python_script, source_python_script_for_objfile):
        Replace call to PyRun_SimpleFile by call to
        python_run_simple_file.

Tested on x86_64-linux, and on x86-windows (with AdaCore's testsuite).

Thanks!
-- 
Joel

---
 gdb/python/python.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4ef5715..5fef62f 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -134,6 +134,38 @@ 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.
+
+   One of the parameters of PyRun_SimpleFile is a FILE *.
+   The problem is that type FILE is extremely system and compiler
+   dependent.  So, unless the Python library has been compiled using
+   the same build environment as GDB, we run the risk of getting
+   a crash due to inconsistencies between the definition used by GDB,
+   and the definition used by Python.  A mismatch can very likely
+   lead to a crash.  This is particularly visible on Windows, where
+   few users would build Python themselves (this is no trivial task
+   on this platform), and thus use binaries built by someone else
+   instead.
+
+   To work around this potential issue, we create the FILE object
+   using Python routines, thus making sure that it is compatible
+   with the Python library.  */
+
+static void
+python_run_simple_file (const char *filename)
+{
+  char *filename_copy;
+  PyObject* python_file;
+  struct cleanup *cleanup;
+
+  filename_copy = xstrdup (filename);
+  cleanup = make_cleanup (xfree, filename_copy);
+  python_file = PyFile_FromString(filename_copy, "r");
+  make_cleanup_py_decref (python_file);
+  PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
+  do_cleanups (cleanup);
+}
 
 /* Given a command_line, return a command string suitable for passing
    to Python.  Lines in the string are separated by newlines.  The
@@ -573,7 +605,7 @@ source_python_script (FILE *stream, const char *file)
 
   /* Note: If an exception occurs python will print the traceback and
      clear the error indicator.  */
-  PyRun_SimpleFile (stream, file);
+  python_run_simple_file (file);
 
   do_cleanups (cleanup);
 }
@@ -917,7 +949,7 @@ source_python_script_for_objfile (struct objfile *objfile,
   cleanups = ensure_python_env (get_objfile_arch (objfile), current_language);
   gdbpy_current_objfile = objfile;
 
-  PyRun_SimpleFile (stream, file);
+  python_run_simple_file (file);
 
   do_cleanups (cleanups);
   gdbpy_current_objfile = NULL;
-- 
1.7.1

RE: [RFC] Crash sourcing Python script on Windows

[Paul_Koning]

>At long last, I forced myself to look into this. I had noticed that "source script.py" did not work on Windows, and resulted in the debugger crashing.  It turns out that the problem came from the fact that I was using a binaries from python.org where the definition of type FILE difers from the definition our compiler uses.  The crash occurs when trying to call PyRun_SimpleFile with a FILE created by GDB.
>
>The approach we took is to create a new FILE descriptor using Python routines, thus making sure that the FILE is compatible with PyRun_SimpleFile.  There were two such calls, so we wrote a wrapper to that function that only takes a filename, and creates the FILE on the fly.  This avoids code duplication.

That sounds like a good solution.  I'm puzzled though -- how did FILE end up being different?  One would expect the right header files to be used, producing correct/consistent results.  If that didn't happen in this case, could it happen for other aspects of the GDB to Python API?

	paul

Re: [RFC] Crash sourcing Python script on Windows

[John Spencer]

On 09/29/2011 03:01 AM, Paul_Koning@Dell.com wrote:
>> At long last, I forced myself to look into this. I had noticed that "source script.py" did not work on Windows, and resulted in the debugger crashing.  It turns out that the problem came from the fact that I was using a binaries from python.org where the definition of type FILE difers from the definition our compiler uses.  The crash occurs when trying to call PyRun_SimpleFile with a FILE created by GDB.
>>
>> The approach we took is to create a new FILE descriptor using Python routines, thus making sure that the FILE is compatible with PyRun_SimpleFile.  There were two such calls, so we wrote a wrapper to that function that only takes a filename, and creates the FILE on the fly.  This avoids code duplication.
> That sounds like a good solution.  I'm puzzled though -- how did FILE end up being different?  One would expect the right header files to be used, producing correct/consistent results.  If that didn't happen in this case, could it happen for other aspects of the GDB to Python API?
>
> 	paul
>
FILE is supposed to be an opaque type and as such noone except of the 
libc which defines it is supposed to "poke" at its internals.
however it is common practice in GNU software to assume everybody uses 
GLIBC and poke around in internal stuff thats not supposed to be 
accessibly at all in userland. another example of such illegal behaviour 
is libevent, which does illegal things with fd_set internals to allow 
more than FD_SETSIZE file descriptors to be used with select().

Re: [RFC] Crash sourcing Python script on Windows

[Joel Brobecker]

> FILE is supposed to be an opaque type and as such noone except of
> the libc which defines it is supposed to "poke" at its internals.
> however it is common practice in GNU software to assume everybody
> uses GLIBC and poke around in internal stuff thats not supposed to
> be accessibly at all in userland.

It could be something simpler than that. Python was built on one
system, using an unknown build environment. when then use that
library to link it against some code on a version of Windows that
is most likely different, with a compiler that is also most likely
different. If each compiler used a libc where the definition of
that type is different, then we have an incompatibility.

-- 
Joel

Re: [RFC] Crash sourcing Python script on Windows

[Eli Zaretskii]

> From: <Paul_Koning@Dell.com>
> Date: Wed, 28 Sep 2011 20:01:20 -0500
> 
> That sounds like a good solution.

I agree, FWIW.  I tried to source a Python script on my Windows box
(with Python26.dll), and sure thing, GDB crashed.

>  I'm puzzled though -- how did FILE end up being different?  One would expect the right header files to be used, producing correct/consistent results.  If that didn't happen in this case, could it happen for other aspects of the GDB to Python API?

Python.org builds Python for Windows using the Microsoft compiler.  (I
have no idea why they use a proprietary compiler when a free one is
available and rock solid.)  By contrast, GDB is built using MinGW
runtime and headers.  The ABI is completely compatible AFAIK, but some
incompatibilities can still lurk, especially in data types internal to
the library.  I agree that it's strange to see such an incompatibility
in the FILE object.  Joel, could you provide some details about this
incompatibility?  Perhaps we could use some GCC switch while building
GDB, to work around this?  (The -mms-bitfields switch comes to min.)

Re: [RFC] Crash sourcing Python script on Windows

[Eli Zaretskii]

> Date: Thu, 29 Sep 2011 04:13:20 +0200
> From: John Spencer <maillist-gdbpatches@barfooze.de>
> 
> FILE is supposed to be an opaque type and as such noone except of the 
> libc which defines it is supposed to "poke" at its internals.
> however it is common practice in GNU software to assume everybody uses 
> GLIBC and poke around in internal stuff thats not supposed to be 
> accessibly at all in userland. another example of such illegal behaviour 
> is libevent, which does illegal things with fd_set internals to allow 
> more than FD_SETSIZE file descriptors to be used with select().

Do we really have such code in GDB?  I'd be surprised.

Re: [RFC] Crash sourcing Python script on Windows

[Eli Zaretskii]

> Date: Wed, 28 Sep 2011 21:06:34 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> It could be something simpler than that. Python was built on one
> system, using an unknown build environment. when then use that
> library to link it against some code on a version of Windows that
> is most likely different, with a compiler that is also most likely
> different. If each compiler used a libc where the definition of
> that type is different, then we have an incompatibility.

That's true, but just looking at the definitions of the FILE object in
both MS and MinGW headers, I can't see any differences.  Which is
quite expected: if it were not so, MinGW programs couldn't call APIs
that accept FILE objects, like `fopen', since the implementation of
`fopen' is in the Microsoft runtime DLLs, which was compiled by the
Microsoft compiler, the same one used to build Python DLLs for
Windows.

So it would be good to know the details of what led you to the
conclusion that the FILE object was incompatible between GDB and
Python DLL.

Re: [RFC] Crash sourcing Python script on Windows

[Kai Tietz]

2011/9/29 Eli Zaretskii <eliz@gnu.org>:
>> Date: Wed, 28 Sep 2011 21:06:34 -0700
>> From: Joel Brobecker <brobecker@adacore.com>
>> Cc: gdb-patches@sourceware.org
>>
>> It could be something simpler than that. Python was built on one
>> system, using an unknown build environment. when then use that
>> library to link it against some code on a version of Windows that
>> is most likely different, with a compiler that is also most likely
>> different. If each compiler used a libc where the definition of
>> that type is different, then we have an incompatibility.
>
> That's true, but just looking at the definitions of the FILE object in
> both MS and MinGW headers, I can't see any differences.  Which is
> quite expected: if it were not so, MinGW programs couldn't call APIs
> that accept FILE objects, like `fopen', since the implementation of
> `fopen' is in the Microsoft runtime DLLs, which was compiled by the
> Microsoft compiler, the same one used to build Python DLLs for
> Windows.

The issue here is not the definition of the FILE type itself in this
case.  The problem is that python (if built with VC) uses different
runtime-version (msvcr100.dll or lower), but mingw uses by well know
reasons the msvcrt.dll.  So by loading this python-DLL into a
mingw-built executable, we have two different runtimes at the same
time.  So FILE * of one library not necessarily operates in the other
library.  By the way, the same issue can lead also for different
C-runtime functions pretty funny results.

Regards,
Kai

Re: [RFC] Crash sourcing Python script on Windows

[Eli Zaretskii]

> Date: Thu, 29 Sep 2011 10:38:51 +0200
> From: Kai Tietz <ktietz70@googlemail.com>
> Cc: Joel Brobecker <brobecker@adacore.com>, maillist-gdbpatches@barfooze.de, 
> 	gdb-patches@sourceware.org
> 
> The issue here is not the definition of the FILE type itself in this
> case.  The problem is that python (if built with VC) uses different
> runtime-version (msvcr100.dll or lower), but mingw uses by well know
> reasons the msvcrt.dll.  So by loading this python-DLL into a
> mingw-built executable, we have two different runtimes at the same
> time.

Thanks, this makes sense.

What are the reasons that we build GDB against msvcrt.dll?

> By the way, the same issue can lead also for different C-runtime
> functions pretty funny results.

Exactly.  If we have such an incompatibility once python's DLL is
loaded, it can lead to similar crashes elsewhere.  So, unless we can
find a way of linking against msvcrNNN.dll or build our own Python
with MinGW, it sounds like using Python on Windows is unsafe?

Re: [RFC] Crash sourcing Python script on Windows

[Kai Tietz]

2011/9/29 Eli Zaretskii <eliz@gnu.org>:
>> Date: Thu, 29 Sep 2011 10:38:51 +0200
>> From: Kai Tietz <ktietz70@googlemail.com>
>> Cc: Joel Brobecker <brobecker@adacore.com>, maillist-gdbpatches@barfooze.de,
>>       gdb-patches@sourceware.org
>>
>> The issue here is not the definition of the FILE type itself in this
>> case.  The problem is that python (if built with VC) uses different
>> runtime-version (msvcr100.dll or lower), but mingw uses by well know
>> reasons the msvcrt.dll.  So by loading this python-DLL into a
>> mingw-built executable, we have two different runtimes at the same
>> time.
>
> Thanks, this makes sense.
>
> What are the reasons that we build GDB against msvcrt.dll?

The issue is a legal thing.  The newer runtimes mvcrXX.dlls aren't
part of the OS itself.  The msvcrt.dll is present on all WIndows OSes
(well always in a different version).  So we can assume that everybody
has a working msvcrt.dll present on his OS.  Due the fact that
msvcrXX.dll aren't part of runtime, you would need to distribute those
DLLs to end-user, which would be (INAL) a violation of EULA as far as
I understood it, if you haven't a VC license for it.

>> By the way, the same issue can lead also for different C-runtime
>> functions pretty funny results.
>
> Exactly.  If we have such an incompatibility once python's DLL is
> loaded, it can lead to similar crashes elsewhere.  So, unless we can
> find a way of linking against msvcrNNN.dll or build our own Python
> with MinGW, it sounds like using Python on Windows is unsafe?

Well, not necessarily.  You need just to take care that file-I/O and
memory-functions are used only in context of one of those libraries
and no exchange of those pointers/file-handles (on C-level, not
OS-level) happens.  For example Java-VM-Runtime works that way pretty
nice with msvcrt, as long as you take care not to do here those
failures.

Regards,
Kai

Re: [RFC] Crash sourcing Python script on Windows

[Pedro Alves]

On Thursday 29 September 2011 05:06:34, Joel Brobecker wrote:
> > FILE is supposed to be an opaque type and as such noone except of
> > the libc which defines it is supposed to "poke" at its internals.
> > however it is common practice in GNU software to assume everybody
> > uses GLIBC and poke around in internal stuff thats not supposed to
> > be accessibly at all in userland.
> 
> It could be something simpler than that. Python was built on one
> system, using an unknown build environment. when then use that
> library to link it against some code on a version of Windows that
> is most likely different, with a compiler that is also most likely
> different. If each compiler used a libc where the definition of
> that type is different, then we have an incompatibility.

It is much more likely that your python is linked with a C runtime,
while gdb is linked with another.  Try "info sharedlibrary",
and you'll probably see both msvcrXX.dll and both msvcrt.dll loaded.
There's not a single/main C runtime on a Windows system.  The C runtime on
Windows is not a central part of the whole system runtime like tradicionally
on Unix.  The NT api (and Win32 on top) fills that central role.

There's msvcrt.dll, originally part of Visual C++, but which is bundled
in Windows nowadays (*), and then there's the msvcrtd.dll, the debug variant,
and then Visual Studio will default to link with a different C runtime, and
a different one for each version, going by msvcrXXX.dll names (with XXX
being 70, 80, 90, 100, etc.)

It is common to end up with more than one C runtime loaded
on Windows.  Windows dlls are self contained, and all its symbols
must be defined at link-time.  There's no symbol interposition
concept in PE.

If you have your application linked with, say, msvcrt.dll, which is
what mingw links with by default, and you load a Dll that was linked
with, say, msvcr70.dll, you'll end up with both C runtimes loaded
in the process.  Each of the runtimes will have its own separate
internal state.  That's the crux.  Given that, you need to be careful to
not pass/use C runtime objects across dlls.  E.g., if you malloc 
in dll/executable A, you can pass that around to other dlls, but you should
`free' that memory back on dll/executable A, so that the proper runtime
is called.  If you had free'd memory in dll B instead (which was linked
against a different runtime, hence you call a different `free' function), B's
C runtime would have no clue what that pointer was -- it's just gargabe from
its perpective, and you'd likely crash.

Same with FILE*, and file descriptions.  Each runtime maintains its
own internal state for those.

In a nutshell, your patch is correct.

If such a fix is not always possible, then we'll have to make sure you link
the same runtime in all the offending dlls (those that pass around C
runtime objects).

-- 
Pedro Alves

Re: [RFC] Crash sourcing Python script on Windows

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Thu, 29 Sep 2011 11:51:30 +0100
> Cc: Joel Brobecker <brobecker@adacore.com>, John Spencer <maillist-gdbpatches@barfooze.de>
> 
> Try "info sharedlibrary", and you'll probably see both msvcrXX.dll
> and both msvcrt.dll loaded.

I get an "No shared libraries loaded at this time." response to that
command.  If I load a program and let it start, then I see only
msvcrt.dll, even after issuing a Python command, like "python print 23".

Am I doing something wrong?

Re: [RFC] Crash sourcing Python script on Windows

[Kai Tietz]

2011/9/29 Eli Zaretskii <eliz@gnu.org>:
>> From: Pedro Alves <pedro@codesourcery.com>
>> Date: Thu, 29 Sep 2011 11:51:30 +0100
>> Cc: Joel Brobecker <brobecker@adacore.com>, John Spencer <maillist-gdbpatches@barfooze.de>
>>
>> Try "info sharedlibrary", and you'll probably see both msvcrXX.dll
>> and both msvcrt.dll loaded.
>
> I get an "No shared libraries loaded at this time." response to that
> command.  If I load a program and let it start, then I see only
> msvcrt.dll, even after issuing a Python command, like "python print 23".
>
> Am I doing something wrong?

Not necessarily.  It might be that Python uses static runtime library.

Regards,
Kai

Re: [RFC] Crash sourcing Python script on Windows

[Pedro Alves]

On Thursday 29 September 2011 13:32:06, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Thu, 29 Sep 2011 11:51:30 +0100
> > Cc: Joel Brobecker <brobecker@adacore.com>, John Spencer <maillist-gdbpatches@barfooze.de>
> > 
> > Try "info sharedlibrary", and you'll probably see both msvcrXX.dll
> > and both msvcrt.dll loaded.
> 
> I get an "No shared libraries loaded at this time." response to that
> command.  If I load a program and let it start, then I see only
> msvcrt.dll, even after issuing a Python command, like "python print 23".
> 
> Am I doing something wrong?

I meant, debug gdb under gdb, and do "(top-gdb) info sharedlibrary"
to see what libraries have been loaded by the inferior gdb.
It sounds like you loaded some other program that is not gdb?

-- 
Pedro Alves

Re: [RFC] Crash sourcing Python script on Windows

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>         * python/python.c (python_run_simple_file): New function.
Joel>         (source_python_script, source_python_script_for_objfile):
Joel>         Replace call to PyRun_SimpleFile by call to
Joel>         python_run_simple_file.

It seems good to me.  Some trivial nits follow.

Joel> +/* A wrapper around PyRun_SimpleFile.  FILENAME is the name of
Joel> +   the Python script to run.
Joel> +
Joel> +   One of the parameters of PyRun_SimpleFile is a FILE *.
Joel> +   The problem is that type FILE is extremely system and compiler
Joel> +   dependent.  So, unless the Python library has been compiled using
Joel> +   the same build environment as GDB, we run the risk of getting
Joel> +   a crash due to inconsistencies between the definition used by GDB,
Joel> +   and the definition used by Python.  A mismatch can very likely
Joel> +   lead to a crash.  This is particularly visible on Windows, where
Joel> +   few users would build Python themselves (this is no trivial task
Joel> +   on this platform), and thus use binaries built by someone else
Joel> +   instead.

It would be good if this mentioned the specific case of linking against
different versions of the C runtime.

Joel> +  PyObject* python_file;

Wrong "*" positioning.

Joel> +  python_file = PyFile_FromString(filename_copy, "r");

Missing space before "(".

thanks,
Tom

Re: [RFC] Crash sourcing Python script on Windows

[Tom Tromey]

>>>>> "John" == John Spencer <maillist-gdbpatches@barfooze.de> writes:

John> FILE is supposed to be an opaque type and as such noone except of the
John> libc which defines it is supposed to "poke" at its internals.

Which is exactly what GDB does.

John> however it is common practice in GNU software to assume everybody uses
John> GLIBC and poke around in internal stuff thats not supposed to be
John> accessibly at all in userland. another example of such illegal
John> behaviour is libevent, which does illegal things with fd_set internals
John> to allow more than FD_SETSIZE file descriptors to be used with
John> select().

I don't even know what libevent is.  It isn't part of gdb.

I'm guessing you are referring back to the libthread_db discussion.
If so, please separate the streams.  Thanks.

Tom

Re: [RFC] Crash sourcing Python script on Windows

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

> Joel>         * python/python.c (python_run_simple_file): New function.
> Joel>         (source_python_script, source_python_script_for_objfile):
> Joel>         Replace call to PyRun_SimpleFile by call to
> Joel>         python_run_simple_file.
> 
> It seems good to me.  Some trivial nits follow.

Thanks for the review.

> It would be good if this mentioned the specific case of linking against
> different versions of the C runtime.

Attached is what I checked in. The paragraph on Windows is plagiarism
on what Kai said, so credits to him.  Let me know if you see some nits,
or if this wasn't what you had in mind, and I will fix them.

-- 
Joel

[-- Attachment #2: source-python-windows.diff --]
[-- Type: text/x-diff, Size: 3596 bytes --]

commit b783e81f56f6e4d4eeec839cd83084db83b8a787
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Mon Sep 26 17:55:04 2011 -0700

    Crash sourcing Python script on Windows
    
    gdb/ChangeLog:
    
            * python/python.c (python_run_simple_file): New function.
            (source_python_script, source_python_script_for_objfile):
            Replace call to PyRun_SimpleFile by call to
            python_run_simple_file.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 625bbbb..de4fdc8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2011-10-03  Joel Brobecker  <brobecker@adacore.com>
+
+	* python/python.c (python_run_simple_file): New function.
+	(source_python_script, source_python_script_for_objfile):
+	Replace call to PyRun_SimpleFile by call to
+	python_run_simple_file.
+
 2011-10-03  Paul Koning  <paul_koning@dell.com>
 
 	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4ef5715..61c5420 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -134,6 +134,45 @@ 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.
+
+   One of the parameters of PyRun_SimpleFile is a FILE *.
+   The problem is that type FILE is extremely system and compiler
+   dependent.  So, unless the Python library has been compiled using
+   the same build environment as GDB, we run the risk of getting
+   a crash due to inconsistencies between the definition used by GDB,
+   and the definition used by Python.  A mismatch can very likely
+   lead to a crash.
+
+   There is also the situation where the Python library and GDB
+   are using two different versions of the C runtime library.
+   This is particularly visible on Windows, where few users would
+   build Python themselves (this is no trivial task on this platform),
+   and thus use binaries built by someone else instead. Python,
+   being built with VC, would use one version of the msvcr DLL
+   (Eg. msvcr100.dll), while MinGW uses msvcrt.dll.  A FILE *
+   from one runtime does not necessarily operate correctly in
+   the other runtime.
+
+   To work around this potential issue, we create the FILE object
+   using Python routines, thus making sure that it is compatible
+   with the Python library.  */
+
+static void
+python_run_simple_file (const char *filename)
+{
+  char *filename_copy;
+  PyObject *python_file;
+  struct cleanup *cleanup;
+
+  filename_copy = xstrdup (filename);
+  cleanup = make_cleanup (xfree, filename_copy);
+  python_file = PyFile_FromString (filename_copy, "r");
+  make_cleanup_py_decref (python_file);
+  PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
+  do_cleanups (cleanup);
+}
 
 /* Given a command_line, return a command string suitable for passing
    to Python.  Lines in the string are separated by newlines.  The
@@ -573,7 +612,7 @@ source_python_script (FILE *stream, const char *file)
 
   /* Note: If an exception occurs python will print the traceback and
      clear the error indicator.  */
-  PyRun_SimpleFile (stream, file);
+  python_run_simple_file (file);
 
   do_cleanups (cleanup);
 }
@@ -917,7 +956,7 @@ source_python_script_for_objfile (struct objfile *objfile,
   cleanups = ensure_python_env (get_objfile_arch (objfile), current_language);
   gdbpy_current_objfile = objfile;
 
-  PyRun_SimpleFile (stream, file);
+  python_run_simple_file (file);
 
   do_cleanups (cleanups);
   gdbpy_current_objfile = NULL;

Re: [RFC] Crash sourcing Python script on Windows

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> It would be good if this mentioned the specific case of linking against
Tom> different versions of the C runtime.

Joel> Attached is what I checked in. The paragraph on Windows is plagiarism
Joel> on what Kai said, so credits to him.  Let me know if you see some nits,
Joel> or if this wasn't what you had in mind, and I will fix them.

Looks great, thanks.

Tom

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

[Jan Kratochvil]

Hello Joel,

On Thu, 29 Sep 2011 01:19:56 +0200, Joel Brobecker wrote:
> As a result, I decided
> to apply the workaround unconditionally on all platforms.
> I don't see this as a big performance impact in practice.

follow-up to:
	[patch] Code cleanup-like: Do not open Python scripts twice
	http://sourceware.org/ml/gdb-patches/2012-01/msg00774.html

While I agree it was not such a big degradation of GDB I do not find
acceptable degrading normal platforms just due to MS-Windows defects.

Could you verify this testcase really catches the crashing situation, please?
(autoheader + autoconf required)

No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.


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,46 @@ aix*)
   ;;
 esac
 
+# MS-Windows platform may have Python compiled with different libc
+# having the FILE structure layout incompatible between libraries.
+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,

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

[Pedro Alves]

On 01/23/2012 06:11 PM, Jan Kratochvil wrote:
> Hello Joel,
> 
> On Thu, 29 Sep 2011 01:19:56 +0200, Joel Brobecker wrote:
>> As a result, I decided
>> to apply the workaround unconditionally on all platforms.
>> I don't see this as a big performance impact in practice.
> 
> follow-up to:
> 	[patch] Code cleanup-like: Do not open Python scripts twice
> 	http://sourceware.org/ml/gdb-patches/2012-01/msg00774.html
> 
> While I agree it was not such a big degradation of GDB I do not find
> acceptable degrading normal platforms just due to MS-Windows defects.

Agreed.  To be honest, it's not a defect, it's a personality trait...

> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1566,6 +1566,46 @@ aix*)
>    ;;
>  esac
>  
> +# 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.


> +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 ());

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

The patch looks good to me.

-- 
Pedro Alves

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

[Pedro Alves]

Does anyone know if there's a way to create a PyFile from an open Windows
file HANDLE (that is, an hypothetical PyFile_from_HANDLE function)?

If there is, we could do something like:

 static PyObject *
 PyFile_from_FILE (FILE *file, const char *filename, char *mode)
 {
 #ifdef _WIN32
   HANDLE h = _get_osfhandle (fileno (file));
   return PyFile_from_HANDLE (h, ...);
 #else
   return PyFile_FromFile(file, filename, mode, ...);
 }

Unlike libc objects, HANDLEs are common to all loaded libcs, given
that they live in the common Win32/NT layer beneath libc, so that
would work around the multiple libc's problem, and, would plug the
race on Windows too.

_get_osfhandle:
http://msdn.microsoft.com/en-us/library/ks2530z6.aspx

-- 
Pedro Alves

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

[Jan Kratochvil]

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,

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

[Doug Evans]

On Mon, Jan 23, 2012 at 1:08 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> 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.

Hi.
Do you have any data that shows there is a problem with the double opening?
[I'm kinda uncomfortable with this patch, as is.]

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

[Jan Kratochvil]

Hello Doug,

On Mon, 23 Jan 2012 22:59:58 +0100, Doug Evans wrote:
> Do you have any data that shows there is a problem with the double opening?

Native GNU/Linux application would never double open files.

It is the similar reason I disable GDB directories relocation in Fedora - as
it is not a normal native application behavior - and in some corner cases it
even breaks execution.

GDB is still a bit exotic codebase nowadays, there is a long way to make it
a normal application with codebase for easy contributions:
	http://sourceware.org/gdb/wiki/ProjectIdeas
		Here are some specific internal cleanups that are worth doing:


> [I'm kinda uncomfortable with this patch, as is.]

I am uncomfortable with code opening file by filename I have already an opened
FILE * for, that is IMNSHO a clear bug.

But sure if there is consensus it cannot be fixed in FSF GDB I can move the
fix to Fedora GDB, there is now already ~50 to-be-merged patches.


Thanks,
Jan

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

[Doug Evans]

On Mon, Jan 23, 2012 at 2:17 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hello Doug,
>
> On Mon, 23 Jan 2012 22:59:58 +0100, Doug Evans wrote:
>> Do you have any data that shows there is a problem with the double opening?
>
> Native GNU/Linux application would never double open files.

I dunno about "never", but I understand the gist of the point.

> It is the similar reason I disable GDB directories relocation in Fedora - as
> it is not a normal native application behavior - and in some corner cases it
> even breaks execution.

Huh.  Can you elaborate?

> GDB is still a bit exotic codebase nowadays, there is a long way to make it
> a normal application with codebase for easy contributions:
>        http://sourceware.org/gdb/wiki/ProjectIdeas
>                Here are some specific internal cleanups that are worth doing:

I don't understand how this is dispositive to this thread.

>> [I'm kinda uncomfortable with this patch, as is.]
>
> I am uncomfortable with code opening file by filename I have already an opened
> FILE * for, that is IMNSHO a clear bug.
>
> But sure if there is consensus it cannot be fixed in FSF GDB I can move the
> fix to Fedora GDB, there is now already ~50 to-be-merged patches.

Only 50?  It's getting better then.  1/2 :-)

btw, my main concern is the nature of the test on windows, and hoping
there is a better way to do this.

For reference sake, and I don't know if/when we'll switch to 3.x, or
support 2.x and 3.x, but Python 3.x uses fds not FILE*s.
[PyFile_FromFile is gone, and 3.x has PyFile_FromFd]

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

[Jan Kratochvil]

On Mon, 23 Jan 2012 23:47:12 +0100, Doug Evans wrote:
> On Mon, Jan 23, 2012 at 2:17 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > It is the similar reason I disable GDB directories relocation in Fedora - as
> > it is not a normal native application behavior - and in some corner cases it
> > even breaks execution.
> 
> Huh.  Can you elaborate?

# ls -l /var/mail/root /var/lib/mock/fedora-15-x86_64/root/var/mail/root
-rw------- 1 root root 785 Jan 24 00:08 /var/mail/root
-rw------- 1 root root   0 Jan 24 15:06 /var/lib/mock/fedora-15-x86_64/root/var/mail/root
# mutt
 --- The mail (785 bytes) is displayed correctly.
# /var/lib/mock/fedora-15-x86_64/root/usr/bin/mutt
 --- The mail (785 bytes) is also displayed correctly.

# gdb mkdir
GNU gdb (GDB) Fedora (7.3.50.20110722-10.fc16)
[...]
Reading symbols from /bin/mkdir...Reading symbols from /usr/lib/debug/bin/mkdir.debug...done.
done.
 --- GDB works OK
# /var/lib/mock/fedora-15-x86_64/root/usr/bin/gdb mkdir
GNU gdb (GDB) Fedora (7.3.1-47.fc15)
[...]
Reading symbols from /bin/mkdir...Missing separate debuginfo for /bin/mkdir
Try: yum --disablerepo='*' --enablerepo='*-debuginfo' install /var/lib/mock/fedora-15-x86_64/root/usr/lib/debug/.build-id/93/d9b3a3ab008848c7cb73979e6d7a6bb1256e28.debug
(no debugging symbols found)...done.
 --- GDB is broken!  Why it reads some weird paths?  I just ran the gdb binary.
 --- These GDB variants here still contain the FSF GDB directory relocations.
 vvv
(gdb) python print gdb.PYTHONDIR
/var/lib/mock/fedora-15-x86_64/root/usr/share/gdb/python
(gdb) show debug-file-directory
The directory where separate debug symbols are searched for is "/var/lib/mock/fedora-15-x86_64/root/usr/lib/debug".
# ldd /var/lib/mock/fedora-15-x86_64/root/usr/bin/gdb
	libpython2.7.so.1.0 => /usr/lib64/libpython2.7.so.1.0 (0x0000003690c00000)
 --- --disable-rpath is in use: http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath

That is broken in all ways.  It is neither the host F-16 system nor the guest
F-15 system.  It uses scripts built for F-15 Python with F-16 Python.
It tries to use debuginfos from F-15 guest with F-16 host.

All I asked was to run the "gdb" binary.  If I wanted to change the debuginfo
path or LD_LIBRARY_PATH etc. I could ask for or even use chroot but I did not.

And it does not make sense to discuss what is better or worse, it is only
important the standard is to not do directories relocation.  There exist
enough system features (chroot, LD_LIBRARY_PATH etc.) to do arbitrary
directory relocations when one needs to.  All the Python, debuginfo etc. paths
are already tricky enough which I know from the GDB Fedora packaging issues.
According to recent #gdb discussion Debian still does not have working pretty
printers after 3 years, the directories difficulties may be one of the
reasons.  If the tools try to even "randomly" modify the directories on their
own each run it is a troubleshooting nightmare.


> > GDB is still a bit exotic codebase nowadays, there is a long way to make it
> > a normal application with codebase for easy contributions:
> >        http://sourceware.org/gdb/wiki/ProjectIdeas
> >                Here are some specific internal cleanups that are worth doing:
> 
> I don't understand how this is dispositive to this thread.

For example the point I added recently:
	GDB/binutils no longer support pre-ANSI C compilers. But
	include/ansidecl.h still contains support for #define PTR char * and
	other K&R statements. Substitute these obsolete macros everywhere for
	their ANSI C variant and remove their definition. Check als the GCC
	source tree using include/ansidecl.h. 

That xmalloc returns PTR (and not void * like malloc does) and that PTR can be
char * in one #ifdef path in include/ansidecl.h and that some GDB code really
casts all the xmalloc results together with archer-ratmice-compile-Wc++-compat
and switches of GDB to C++ discussions where in C++ one does need to cast void
* create a maze of possibilities what is the right way to allocate memory
I had difficulties even just allocating a memory in GDB as a newbie.

How easier would be if xmalloc just returned void * and the GDB codebase did
not case xmalloc results everywhere, like every other package does (using
either malloc directly or some NULL-checking equivalent instead).


I find casting xmalloc results the same code mystery making contributions
difficult like when one sees in code open of a file by filename while the code
has its FILE * at hand.  Both cases indicate I do not understand the code I am
trying contribute to.


> > But sure if there is consensus it cannot be fixed in FSF GDB I can move the
> > fix to Fedora GDB, there is now already ~50 to-be-merged patches.
> 
> Only 50?  It's getting better then.  1/2 :-)

It was 55 in 2010, it is only 41 now, it is a real a progress (sure it is
thanks to FSF GDB progress by all the contributors making just some of the
Fedora patches obsolete, not just by upstreaming them myself).


> btw, my main concern is the nature of the test on windows, and hoping
> there is a better way to do this.

I do not think MS-Windows can be fixed so I find right to have correct code on
normal platforms and best-effort on MS-Windows.  Sure I am open to better
workarounds of MS-Windows but I do not have the platform available
+ willingness to spend more than required-for-GDB time on that platform
myself.


> For reference sake, and I don't know if/when we'll switch to 3.x, or
> support 2.x and 3.x, but Python 3.x uses fds not FILE*s.
> [PyFile_FromFile is gone, and 3.x has PyFile_FromFd]

fd vs. FILE * I find a coding detail.  I find important the point of never
regressing GDB on correctly behaving platform just due to some other broken
platform.


Thanks,
Jan

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

[Pedro Alves]

On 01/23/2012 10:47 PM, Doug Evans wrote:
> For reference sake, and I don't know if/when we'll switch to 3.x, or
> support 2.x and 3.x, but Python 3.x uses fds not FILE*s.
> [PyFile_FromFile is gone, and 3.x has PyFile_FromFd]

Unfortunately, that doesn't actually fix the problem.  File descriptors are still
private per C run time.

-- 
Pedro Alves

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

[Doug Evans]

On Tue, Jan 24, 2012 at 6:36 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 23 Jan 2012 23:47:12 +0100, Doug Evans wrote:
>> On Mon, Jan 23, 2012 at 2:17 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
>> > It is the similar reason I disable GDB directories relocation in Fedora - as
>> > it is not a normal native application behavior - and in some corner cases it
>> > even breaks execution.
>>
>> Huh.  Can you elaborate?
>
> # ls -l /var/mail/root /var/lib/mock/fedora-15-x86_64/root/var/mail/root
> -rw------- 1 root root 785 Jan 24 00:08 /var/mail/root
> -rw------- 1 root root   0 Jan 24 15:06 /var/lib/mock/fedora-15-x86_64/root/var/mail/root
> # mutt
>  --- The mail (785 bytes) is displayed correctly.
> # /var/lib/mock/fedora-15-x86_64/root/usr/bin/mutt
>  --- The mail (785 bytes) is also displayed correctly.
>
> # gdb mkdir
> GNU gdb (GDB) Fedora (7.3.50.20110722-10.fc16)
> [...]
> Reading symbols from /bin/mkdir...Reading symbols from /usr/lib/debug/bin/mkdir.debug...done.
> done.
>  --- GDB works OK
> # /var/lib/mock/fedora-15-x86_64/root/usr/bin/gdb mkdir
> GNU gdb (GDB) Fedora (7.3.1-47.fc15)
> [...]
> Reading symbols from /bin/mkdir...Missing separate debuginfo for /bin/mkdir
> Try: yum --disablerepo='*' --enablerepo='*-debuginfo' install /var/lib/mock/fedora-15-x86_64/root/usr/lib/debug/.build-id/93/d9b3a3ab008848c7cb73979e6d7a6bb1256e28.debug
> (no debugging symbols found)...done.
>  --- GDB is broken!  Why it reads some weird paths?  I just ran the gdb binary.
>  --- These GDB variants here still contain the FSF GDB directory relocations.
>  vvv
> (gdb) python print gdb.PYTHONDIR
> /var/lib/mock/fedora-15-x86_64/root/usr/share/gdb/python
> (gdb) show debug-file-directory
> The directory where separate debug symbols are searched for is "/var/lib/mock/fedora-15-x86_64/root/usr/lib/debug".
> # ldd /var/lib/mock/fedora-15-x86_64/root/usr/bin/gdb
>        libpython2.7.so.1.0 => /usr/lib64/libpython2.7.so.1.0 (0x0000003690c00000)
>  --- --disable-rpath is in use: http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath

I can understand not wanting debug-file-directory to "move" in this
particular case, but I wouldn't necessarily want to remove the
relocatability.
[IOW, remembering that debug-file-directory is misnamed, sigh, it can
have multiple directories.  Fedora could have a system-wide debug file
directory that didn't move and one tied to gdb's installation
directory that did move (and, obviously, remove dups).] OTOH, what's
in this directory is less specific to the gdb being run, it makes less
sense for this directory to be relocatable.  OTOOH, changing things
may break something, so if you wanted to add
--with-system-separate-debug-dir or a way to specify
--with-separate-debug-dir that wasn't relocatable (even though it
otherwise would be), or some such, I think that'd be ok.

As for PYTHONDIR, what's in this directory is (supposed to be)
particular to that gdb.
I've been thinking IWBN to have another directory for things not
specific to a particular gdb, but python already has a "site"
directory for such things ("site-packages" or some such) so I haven't
pursued it.

> [...] It uses scripts built for F-15 Python with F-16 Python.

Rather, it's using scripts for F-15 GDB with F-16 Python, because
that's what you asked for.
If you want to run F-15 GDB with F-15 python, then do that ...
[and if there are problems there, fix 'em; but they're not because
F-15 gdb is using its own scripts]

> It tries to use debuginfos from F-15 guest with F-16 host.
>
> All I asked was to run the "gdb" binary.  If I wanted to change the debuginfo
> path or LD_LIBRARY_PATH etc. I could ask for or even use chroot but I did not.
>
> And it does not make sense to discuss what is better or worse, it is only
> important the standard is to not do directories relocation.

Well, the important thing is to pick what works for the right reasons,
not introduce unwarranted incompatibilities, etc.

> There exist
> enough system features (chroot, LD_LIBRARY_PATH etc.) to do arbitrary
> directory relocations when one needs to.

[AIUC] I'd instead impose requiring chroot on Fedora package testers
than regular users.

> All the Python, debuginfo etc. paths
> are already tricky enough which I know from the GDB Fedora packaging issues.
> According to recent #gdb discussion Debian still does not have working pretty
> printers after 3 years, the directories difficulties may be one of the
> reasons.  If the tools try to even "randomly" modify the directories on their
> own each run it is a troubleshooting nightmare.

These are qualitative assessments that I can't really address ... I
could label almost anything in computerdom as "tricky".
If Debian still doesn't have working pretty-printers, well they're
smart people, I'd like to hear more details ...
I wouldn't want to base decisions on "[...] *may* be one of the
reasons." [emphasis added]

>
>> > GDB is still a bit exotic codebase nowadays, there is a long way to make it
>> > a normal application with codebase for easy contributions:
>> >        http://sourceware.org/gdb/wiki/ProjectIdeas
>> >                Here are some specific internal cleanups that are worth doing:
>>
>> I don't understand how this is dispositive to this thread.
>
> For example the point I added recently:
>        GDB/binutils no longer support pre-ANSI C compilers. But
>        include/ansidecl.h still contains support for #define PTR char * and
>        other K&R statements. Substitute these obsolete macros everywhere for
>        their ANSI C variant and remove their definition. Check als the GCC
>        source tree using include/ansidecl.h.
>
> That xmalloc returns PTR (and not void * like malloc does) and that PTR can be
> char * in one #ifdef path in include/ansidecl.h and that some GDB code really
> casts all the xmalloc results together with archer-ratmice-compile-Wc++-compat
> and switches of GDB to C++ discussions where in C++ one does need to cast void
> * create a maze of possibilities what is the right way to allocate memory
> I had difficulties even just allocating a memory in GDB as a newbie.
>
> How easier would be if xmalloc just returned void * and the GDB codebase did
> not case xmalloc results everywhere, like every other package does (using
> either malloc directly or some NULL-checking equivalent instead).
>
>
> I find casting xmalloc results the same code mystery making contributions
> difficult like when one sees in code open of a file by filename while the code
> has its FILE * at hand.  Both cases indicate I do not understand the code I am
> trying contribute to.

That still doesn't help me decide whether this patch should go in (as is).
If you wanted to punt on windows and just always impose a double
open(), I'd be ok with that, for example.

>
>> > But sure if there is consensus it cannot be fixed in FSF GDB I can move the
>> > fix to Fedora GDB, there is now already ~50 to-be-merged patches.
>>
>> Only 50?  It's getting better then.  1/2 :-)
>
> It was 55 in 2010, it is only 41 now, it is a real a progress (sure it is
> thanks to FSF GDB progress by all the contributors making just some of the
> Fedora patches obsolete, not just by upstreaming them myself).
>
>
>> btw, my main concern is the nature of the test on windows, and hoping
>> there is a better way to do this.
>
> I do not think MS-Windows can be fixed so I find right to have correct code on
> normal platforms and best-effort on MS-Windows.  Sure I am open to better
> workarounds of MS-Windows but I do not have the platform available
> + willingness to spend more than required-for-GDB time on that platform
> myself.
>
>
>> For reference sake, and I don't know if/when we'll switch to 3.x, or
>> support 2.x and 3.x, but Python 3.x uses fds not FILE*s.
>> [PyFile_FromFile is gone, and 3.x has PyFile_FromFd]
>
> fd vs. FILE * I find a coding detail.  I find important the point of never
> regressing GDB on correctly behaving platform just due to some other broken
> platform.

That was just a "for reference sake" remember ...

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

[Jan Kratochvil]

On Tue, 24 Jan 2012 19:32:18 +0100, Doug Evans wrote:

Skipped the discussion what can be more statistically useful and rather chose:

> On Tue, Jan 24, 2012 at 6:36 AM, Jan Kratochvil > <jan.kratochvil@redhat.com> wrote:
> > And it does not make sense to discuss what is better or worse, it is only
> > important the standard is to not do directories relocation.
> 
> Well, the important thing is to pick what works for the right reasons,
> not introduce unwarranted incompatibilities, etc.

If you want to patch other 10000 or how many packages so they all use
directory relocations, you are free to do.  I prefer to remove the directory
relocations from the single (maybe there are few of such) differing GDB
package instead.

As a next step of this discussion I can only submit a ticket to FESCo (Fedora
Engineering Steering Committee) to validate whether my Fedora
non-relocatability patch/idea is right or not.


> [AIUC] I'd instead impose requiring chroot on Fedora package testers
> than regular users.

Normal users always use only the system installed GDB.  Anything else is
unsupported.


> That still doesn't help me decide whether this patch should go in (as is).
> If you wanted to punt on windows and just always impose a double
> open(), I'd be ok with that, for example.

Without MS-Windows the Joel's patch would not be invented and we would just
always do single open() like before.  Or I do not understand it now.


Thanks,
Jan

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

[Doug Evans]

On Tue, Jan 24, 2012 at 11:40 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 24 Jan 2012 19:32:18 +0100, Doug Evans wrote:
>
> Skipped the discussion what can be more statistically useful and rather chose:
>
>> On Tue, Jan 24, 2012 at 6:36 AM, Jan Kratochvil > <jan.kratochvil@redhat.com> wrote:
>> > And it does not make sense to discuss what is better or worse, it is only
>> > important the standard is to not do directories relocation.
>>
>> Well, the important thing is to pick what works for the right reasons,
>> not introduce unwarranted incompatibilities, etc.
>
> If you want to patch other 10000 or how many packages so they all use
> directory relocations, you are free to do.  I prefer to remove the directory
> relocations from the single (maybe there are few of such) differing GDB
> package instead.

I don't deserve that. :-(

> As a next step of this discussion I can only submit a ticket to FESCo (Fedora
> Engineering Steering Committee) to validate whether my Fedora
> non-relocatability patch/idea is right or not.
>
>
>> [AIUC] I'd instead impose requiring chroot on Fedora package testers
>> than regular users.
>
> Normal users always use only the system installed GDB.  Anything else is
> unsupported.

Are contexts being conflated here?
I thought we were talking about what should be in FSF GDB, not Fedora GDB.

>> That still doesn't help me decide whether this patch should go in (as is).
>> If you wanted to punt on windows and just always impose a double
>> open(), I'd be ok with that, for example.
>
> Without MS-Windows the Joel's patch would not be invented and we would just
> always do single open() like before.  Or I do not understand it now.

s/impose a double open()/impose a double open() on windows/
and skip the autoconf test to try to decide whether the libcs are compatible
(which as currently written you yourself have "fear[s]" of).
[the word "fear" can suggest far greater seriousness than is warranted
here, but it's your word :-)]
OTOH, if someone comes up with a succinct and solid test, great.

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

[Jan Kratochvil]

On Tue, 24 Jan 2012 23:10:54 +0100, Doug Evans wrote:
> I thought we were talking about what should be in FSF GDB, not Fedora GDB.

I admit I may not know what to imagine as a "generic distro".


> s/impose a double open()/impose a double open() on windows/
> and skip the autoconf test to try to decide whether the libcs are compatible
> (which as currently written you yourself have "fear[s]" of).
> [the word "fear" can suggest far greater seriousness than is warranted
> here, but it's your word :-)]
> OTOH, if someone comes up with a succinct and solid test, great.

Aha, OK, I am fine with that.


Thanks,
Jan

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

[Jan Kratochvil]

On Tue, 24 Jan 2012 23:19:46 +0100, Jan Kratochvil wrote:
> On Tue, 24 Jan 2012 23:10:54 +0100, Doug Evans wrote:
> > s/impose a double open()/impose a double open() on windows/
> > and skip the autoconf test to try to decide whether the libcs are compatible
> > (which as currently written you yourself have "fear[s]" of).
> > [the word "fear" can suggest far greater seriousness than is warranted
> > here, but it's your word :-)]
> > OTOH, if someone comes up with a succinct and solid test, great.
> 
> Aha, OK, I am fine with that.

With the patch for Joel.


Thanks,
Jan


gdb/
2012-01-25  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.
	* 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 !_WIN32.  Update the function comment.
	(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
@@ -539,9 +539,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/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,34 +151,31 @@ 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.
-
-   One of the parameters of PyRun_SimpleFile is a FILE *.
-   The problem is that type FILE is extremely system and compiler
-   dependent.  So, unless the Python library has been compiled using
-   the same build environment as GDB, we run the risk of getting
-   a crash due to inconsistencies between the definition used by GDB,
-   and the definition used by Python.  A mismatch can very likely
-   lead to a crash.
-
-   There is also the situation where the Python library and GDB
-   are using two different versions of the C runtime library.
-   This is particularly visible on Windows, where few users would
-   build Python themselves (this is no trivial task on this platform),
-   and thus use binaries built by someone else instead. Python,
-   being built with VC, would use one version of the msvcr DLL
-   (Eg. msvcr100.dll), while MinGW uses msvcrt.dll.  A FILE *
-   from one runtime does not necessarily operate correctly in
+/* A wrapper around PyRun_SimpleFile.  FILE is the Python script to run
+   named FILENAME.
+
+   On Windows hosts few users would build Python themselves (this is no
+   trivial task on this platform), and thus use binaries built by
+   someone else instead.  There may happen situation where the Python
+   library and GDB are using two different versions of the C runtime
+   library.  Python, being built with VC, would use one version of the
+   msvcr DLL (Eg. msvcr100.dll), while MinGW uses msvcrt.dll.
+   A FILE * from one runtime does not necessarily operate correctly in
    the other runtime.
 
-   To work around this potential issue, we create the FILE object
-   using Python routines, thus making sure that it is compatible
-   with the Python library.  */
+   To work around this potential issue, we create on Windows hosts the
+   FILE object using Python routines, thus making sure that it is
+   compatible with the Python library.  */
 
 static void
-python_run_simple_file (const char *filename)
+python_run_simple_file (FILE *file, const char *filename)
 {
+#ifndef _WIN32
+
+  PyRun_SimpleFile (file, filename);
+
+#else /* _WIN32 */
+
   char *full_path;
   PyObject *python_file;
   struct cleanup *cleanup;
@@ -198,6 +195,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 /* _WIN32 */
 }
 
 /* Given a command_line, return a command string suitable for passing
@@ -620,17 +619,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 +990,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 +1079,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,

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

[Joel Brobecker]

Hi Jan,

> gdb/
> 2012-01-25  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.
> 	* 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 !_WIN32.  Update the function comment.
> 	(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.

The patch looks good to me and I would probably have come up with
the same sort of changes myself. I tested it on x86-windows, and
sourcing Python files still worked perfectly. I also ran it against
AdaCore's testsuite and no regression either.

-- 
Joel

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

[Jan Kratochvil]

Hi Joel,

On Thu, 26 Jan 2012 07:21:03 +0100, Joel Brobecker wrote:
> The patch looks good to me and I would probably have come up with
> the same sort of changes myself. I tested it on x86-windows, and
> sourcing Python files still worked perfectly. I also ran it against
> AdaCore's testsuite and no regression either.

Thanks for the test.  Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-01/msg00219.html

Sorry I did not participate in the initial review.


Regards,
Jan