gdb/testsuite: test for memory leaks in gdb.Inferior.read_memory()
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
For a long time Fedora GDB has carried an out of tree patch which
checks for memory leaks in gdb.Inferior.read_memory(). At one point
in the distant past GDB did have a memory leak in this code, but this
was first fixed in commit:
commit 655e820cf9a039ee55325d9e1f8423796d592b4b
Date: Wed Mar 28 17:38:07 2012 +0000
* python/py-inferior.c (infpy_read_memory): Remove cleanups and
explicitly free 'buffer' on exit paths. Decref 'membuf_object'
before returning.
And the code has changed a lot since then, but the leak is still
fixed. Unfortunately, this commit didn't have any associated tests.
The original Fedora test wasn't really suitable for upstream, it was
reading /proc/PID/... to figure out if there was a leak or not.
However, we already have gdb.python/py-inferior-leak.exp in upstream
GDB, which makes use of the Python tracemalloc module to check for
memory leaks in a corner of the Python API, so I figured it wouldn't
hurt to rewrite the test in the same style.
And so here is a test for a bug which was closed 12 years ago. This
detects if the gdb.Inferior.read_memory() call leaks any memory.
I've tested this by hacking gdbpy_buffer_to_membuf, replacing the last
line which currently looks like this:
return PyMemoryView_FromObject ((PyObject *) membuf_obj.get ());
and instead doing:
return PyMemoryView_FromObject ((PyObject *) membuf_obj.release ());
The use of "release" here will mean we no longer decrement the
reference count on membuf_obj before returning from the function. As
a consequence the membuf_obj will not be garbage collected. With this
hack in place the new test will fail.
The Python script in the new test is mostly a copy&paste from
py-inferior-leak.py with the core changed to do a memory read instead
of inferior creation. I did consider rewriting both tests into a
single file, maybe, py-memory-leak.py, which would make it easier to
add additional similar tests in the future. For now I've held off
doing that, but if this gets merged then I _might_ revisit this idea.
If folk feel that this new test should only be accepted if I do this
rewrite then let me know and I can get that done.
On copyright date ranges: The .exp and .py scripts are new enough for
this commit that I've dated them 2024. The .c source script is lifted
directly from the old Fedora patch, so I've retained the original 2014
start date for that file only.
---
.../gdb.python/py-read-memory-leak.c | 27 ++++++
.../gdb.python/py-read-memory-leak.exp | 44 +++++++++
.../gdb.python/py-read-memory-leak.py | 92 +++++++++++++++++++
3 files changed, 163 insertions(+)
create mode 100644 gdb/testsuite/gdb.python/py-read-memory-leak.c
create mode 100644 gdb/testsuite/gdb.python/py-read-memory-leak.exp
create mode 100644 gdb/testsuite/gdb.python/py-read-memory-leak.py
base-commit: 43a1fffa62060ce640749dcc9fc17058069ccba6
Comments
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
Andrew> However, we already have gdb.python/py-inferior-leak.exp in upstream
Andrew> GDB, which makes use of the Python tracemalloc module to check for
Andrew> memory leaks in a corner of the Python API, so I figured it wouldn't
Andrew> hurt to rewrite the test in the same style.
Andrew> And so here is a test for a bug which was closed 12 years ago.
:)
Andrew> The Python script in the new test is mostly a copy&paste from
Andrew> py-inferior-leak.py with the core changed to do a memory read instead
Andrew> of inferior creation. I did consider rewriting both tests into a
Andrew> single file, maybe, py-memory-leak.py, which would make it easier to
Andrew> add additional similar tests in the future. For now I've held off
Andrew> doing that, but if this gets merged then I _might_ revisit this idea.
Andrew> If folk feel that this new test should only be accepted if I do this
Andrew> rewrite then let me know and I can get that done.
I think it's fine as-is.
Having this be generalized to make it simpler to add new leak tests
might be nice but I don't think it's a requirement.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
new file mode 100644
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014-2024 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+static struct x
+{
+ char unsigned u[4096];
+} x, *px = &x;
+
+int
+main (void)
+{
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,44 @@
+# Copyright (C) 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite. It checks for memory leaks
+# associated with calling gdb.Inferior.read_memory().
+
+load_lib gdb-python.exp
+
+require allow_python_tests
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+ return -1
+}
+
+if ![runto_main] {
+ return -1
+}
+
+# Skip this test if the tracemalloc module is not available.
+if { ![gdb_py_module_available "tracemalloc"] } {
+ unsupported "tracemalloc module not available"
+ return
+}
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+# Source the Python script, this runs the test (which is written
+# completely in Python), and either prints PASS, or throws an
+# exception.
+gdb_test "source ${pyfile}" "PASS" "source python script"
new file mode 100644
@@ -0,0 +1,92 @@
+# Copyright (C) 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import tracemalloc
+import gdb
+
+# A global variable in which we store a reference to the memory buffer
+# returned from gdb.Inferior.read_memory().
+mem_buf = None
+
+
+# A global filters list, we only care about memory allocations
+# originating from this script.
+filters = [tracemalloc.Filter(True, "*" + os.path.basename(__file__))]
+
+
+# Run the test. When CLEAR is True we clear the global INF variable
+# before comparing the before and after memory allocation traces.
+# When CLEAR is False we leave INF set to reference the gdb.Inferior
+# object, thus preventing the gdb.Inferior from being deallocated.
+def test(clear):
+ global filters, mem_buf
+
+ addr = gdb.parse_and_eval("px")
+ inf = gdb.inferiors()[0]
+
+ # Start tracing, and take a snapshot of the current allocations.
+ tracemalloc.start()
+ snapshot1 = tracemalloc.take_snapshot()
+
+ # Read from the inferior, this allocate a memory buffer object.
+ mem_buf = inf.read_memory(addr, 4096)
+
+ # Possibly clear the global INF variable.
+ if clear:
+ mem_buf = None
+
+ # Now grab a second snapshot of memory allocations, and stop
+ # tracing memory allocations.
+ snapshot2 = tracemalloc.take_snapshot()
+ tracemalloc.stop()
+
+ # Filter the snapshots; we only care about allocations originating
+ # from this file.
+ snapshot1 = snapshot1.filter_traces(filters)
+ snapshot2 = snapshot2.filter_traces(filters)
+
+ # Compare the snapshots, this leaves only things that were
+ # allocated, but not deallocated since the first snapshot.
+ stats = snapshot2.compare_to(snapshot1, "traceback")
+
+ # Total up all the allocated things.
+ total = 0
+ for stat in stats:
+ total += stat.size_diff
+ return total
+
+
+# The first time we run this some global state will be allocated which
+# shows up as memory that is allocated, but not released. So, run the
+# test once and discard the result.
+test(True)
+
+# Now run the test twice, the first time we clear our global reference
+# to the memory buffer object, which should allow Python to deallocate
+# the object. The second time we hold onto the global reference,
+# preventing Python from performing the deallocation.
+bytes_with_clear = test(True)
+bytes_without_clear = test(False)
+
+# The bug that used to exist in GDB was that even when we released the
+# global reference the gdb.Inferior object would not be deallocated.
+if bytes_with_clear > 0:
+ raise gdb.GdbError("memory leak when memory buffer should be released")
+if bytes_without_clear == 0:
+ raise gdb.GdbError("memory buffer object is no longer allocated")
+
+# Print a PASS message that the test script can see.
+print("PASS")