[RFA,11/11] Change python_run_simple_file to use gdbpy_ref
Commit Message
This changes python_run_simple_file to use gdbpy_ref and
unique_xmalloc_ptr. Thi fixes a latent bug in this function, where
the error path previously ran the cleanups and then referred to one of
the objects just freed.
2016-11-12 Tom Tromey <tom@tromey.com>
* python/python.c (python_run_simple_file): Use
unique_xmalloc_ptr, gdbpy_ref.
---
gdb/ChangeLog | 5 +++++
gdb/python/python.c | 18 +++++-------------
2 files changed, 10 insertions(+), 13 deletions(-)
Comments
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> + gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);
This is missing a <char>.
I think if there isn't a win32 buildbot instance, I will just drop this
patch, as I have no way to test it.
Tom
On 11/12/2016 09:39 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>
> Tom> + gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);
>
> This is missing a <char>.
> I think if there isn't a win32 buildbot instance, I will just drop this
> patch, as I have no way to test it.
I'd rather not drop it, since we'll need it in order to get rid of cleanups.
I think better would be to push this to a branch, to make it easy
for others to try it, and call out for (at least build-) testing help.
Actually, you don't really need that -- that WIN32 code should work
on Unix as well, it's just not as efficient as the other branch.
So you can just hack/invert the #ifndef _WIN32 locally for testing.
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Actually, you don't really need that -- that WIN32 code should work
Pedro> on Unix as well, it's just not as efficient as the other branch.
Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
What if I just remove the non-win32 code here?
It might be slightly less efficient, but only slightly; with the upside
being that we'd have just a single code path to test.
Tom
On 11/16/2016 06:22 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> Actually, you don't really need that -- that WIN32 code should work
> Pedro> on Unix as well, it's just not as efficient as the other branch.
> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
>
> What if I just remove the non-win32 code here?
> It might be slightly less efficient, but only slightly; with the upside
> being that we'd have just a single code path to test.
That's fine with me.
Thanks,
Pedro Alves
On 11/16/2016 08:10 PM, Pedro Alves wrote:
> On 11/16/2016 06:22 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> Actually, you don't really need that -- that WIN32 code should work
>> Pedro> on Unix as well, it's just not as efficient as the other branch.
>> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
>>
>> What if I just remove the non-win32 code here?
>> It might be slightly less efficient, but only slightly; with the upside
>> being that we'd have just a single code path to test.
>
> That's fine with me.
>
BTW, for completeness, a middle ground solution would be to change
the #ifndef to an if ().
#ifdef _WIN32
# define GDB_PYTHON_OPENS_FILE 1
#else
# define GDB_PYTHON_OPENS_FILE 0
#endif
static void
python_run_simple_file (FILE *file, const char *filename)
{
if (!GDB_PYTHON_OPENS_FILE)
{
PyRun_SimpleFile (file, filename);
}
else
{
... longer path here ...
}
}
That'd get us compile-time testing of both branches everywhere,
without run time penalty (the compiler optimizes out the dead branch).
But as said, making the longer branch the only branch is also
fine with me.
Thanks,
Pedro Alves
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Pedro> Actually, you don't really need that -- that WIN32 code should work
Pedro> on Unix as well, it's just not as efficient as the other branch.
Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
Tom> What if I just remove the non-win32 code here?
Tom> It might be slightly less efficient, but only slightly; with the upside
Tom> being that we'd have just a single code path to test.
I looked into this some more, and I am going to drop the patch after
all.
The main issue is that PyFile_AsFile doesn't exist in Python 3. In
Python 3 it seems that the implementation will have to be more
complicated: it seems necessary to either evaluate Python code (perhaps
a helper in the gdb module's __init__.py); or to read the file in
python_run_simple_file, call the Python parser, and then evaluate the
result.
One of these seems necessary, but it's not really related to this series
and I'm not sure my interest extends to that.
Tom
On 11/18/2016 11:22 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>
> Pedro> Actually, you don't really need that -- that WIN32 code should work
> Pedro> on Unix as well, it's just not as efficient as the other branch.
> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
>
> Tom> What if I just remove the non-win32 code here?
> Tom> It might be slightly less efficient, but only slightly; with the upside
> Tom> being that we'd have just a single code path to test.
>
> I looked into this some more, and I am going to drop the patch after
> all.
>
> The main issue is that PyFile_AsFile doesn't exist in Python 3. In
> Python 3 it seems that the implementation will have to be more
> complicated: it seems necessary to either evaluate Python code (perhaps
> a helper in the gdb module's __init__.py); or to read the file in
> python_run_simple_file, call the Python parser, and then evaluate the
> result.
>
> One of these seems necessary, but it's not really related to this series
> and I'm not sure my interest extends to that.
Curious, that means that nobody is building mingw gdb against
Python 3 then.
I did git blame on that function now, and that point at git commit
4c63965b8, which shows that we used to do the longer path everywhere,
and from the git log it sounds like we don't want to go back.
So looks like we're back to the original suggestion of hacking the
WIN32 condition locally and test against Python 2. If that works,
push it in.
Thanks,
Pedro Alves
@@ -1,5 +1,10 @@
2016-11-12 Tom Tromey <tom@tromey.com>
+ * python/python.c (python_run_simple_file): Use
+ unique_xmalloc_ptr, gdbpy_ref.
+
+2016-11-12 Tom Tromey <tom@tromey.com>
+
* python/py-prettyprint.c (print_stack_unless_memory_error)
(print_string_repr, print_children): Use gdbpy_ref.
@@ -349,25 +349,17 @@ python_run_simple_file (FILE *file, const char *filename)
#else /* _WIN32 */
- char *full_path;
- PyObject *python_file;
- struct cleanup *cleanup;
-
/* Because we have a string for a filename, and are using Python to
open the file, we need to expand any tilde in the path first. */
- full_path = tilde_expand (filename);
- cleanup = make_cleanup (xfree, full_path);
- python_file = PyFile_FromString (full_path, "r");
- if (! python_file)
+ gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);
+ gdbpy_run_events python_file (PyFile_FromString (full_path.get (), "r"));
+ if (python_file == NULL)
{
- do_cleanups (cleanup);
gdbpy_print_stack ();
- error (_("Error while opening file: %s"), full_path);
+ error (_("Error while opening file: %s"), full_path.get ());
}
- make_cleanup_py_decref (python_file);
- PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
- do_cleanups (cleanup);
+ PyRun_SimpleFile (PyFile_AsFile (python_file.get ()), filename);
#endif /* _WIN32 */
}