* Re: [PATCH] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 14:04 [PATCH] gdb/testsuite: add test for memory requirements of gcore Guinevere Larsen
@ 2025-02-24 15:24 ` Tom de Vries
2025-02-24 19:47 ` Guinevere Larsen
2025-02-24 18:21 ` Andrew Burgess
2025-02-25 13:45 ` [PATCH v2] " Guinevere Larsen
2 siblings, 1 reply; 14+ messages in thread
From: Tom de Vries @ 2025-02-24 15:24 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
On 2/24/25 15:04, Guinevere Larsen wrote:
> For a long time, Fedora has been carrying an out-of-tree patch with a
> similar test to the one proposed in this patch, that ensures that the
> memory requirements don't grow with the inferior's memory. It's been
> so long that the context for why this test exists has been lost, but
> it looked like it could be interesting for upstream.
>
> The test runs twice, once with the inferior allocating 4Mb of memory,
> and the other allocating 64Mb. My plan was to find the rate at which
> things increase based on inferior size, and have that tested to ensure
> we're not growing that requirement accidentally, but my testing
> actually showed memory requirements going down as the inferior increases,
> so instead I just hardcoded that we need less than 2Mb for the command,
> and it can be tweaked later if necessary.
I tried out the test-case, and it passed for me, but after doing:
...
$ echo "1" | sudo tee /proc/sys/kernel/yama/ptrace_scope
...
I get:
...
(gdb) attach 29408^M
Attaching to program:
/data/vries/gdb/leap-15-6/build/gdb/testsuite/outputs/gdb.base/gcore-memory-usage/gcore-memory-usage,
process 29408^M
ptrace: Operation not permitted.^M
(gdb) PASS: gdb.base/gcore-memory-usage.exp: 4 Mb: attach 29408
PASS: gdb.base/gcore-memory-usage.exp: 4 Mb: before: Managed to read the
memory usage
gcore
/data/vries/gdb/leap-15-6/build/gdb/testsuite/outputs/gdb.base/gcore-memory-usage/gcore-memory-usage.core^M
You can't do that without a process to debug.^M
(gdb) FAIL: gdb.base/gcore-memory-usage.exp: 4 Mb: create the corefile
...
So, this is missing "require can_spawn_for_attach".
Thanks,
- Tom
> ---
> gdb/testsuite/gdb.base/gcore-memory-usage.c | 37 +++++++
> gdb/testsuite/gdb.base/gcore-memory-usage.exp | 97 +++++++++++++++++++
> 2 files changed, 134 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
>
> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c b/gdb/testsuite/gdb.base/gcore-memory-usage.c
> new file mode 100644
> index 00000000000..514fdb905cf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
> @@ -0,0 +1,37 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2025 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/>. */
> +
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int main(int argc, char **argv)
> +{
> + /* Use argv to calculate how many megabytes to allocate.
> + Do this to see how much gcore memory usage increases
> + based on inferior dynamic memory. */
> + int megs = atoi (argv[1]);
> + char *p = malloc (megs * 1024 * 1024);
> +
> + /* Wait a long time so GDB can attach to this. */
> + sleep (10);
> +
> + /* Let's be nice citizens :). */
> + free (p);
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
> new file mode 100644
> index 00000000000..cdb6d73c11b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
> @@ -0,0 +1,97 @@
> +# Copyright (C) 2025 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/>.
> +
> +# Test that gcore doesn't end up using excessive memory.
> +
> +require {istarget "*-*-linux*"}
> +
> +standard_testfile
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug}] == -1} {
> + return -1
> +}
> +
> +# Read the proc_pid_status page, to find how much memory the given
> +# PID is using. This is meant to be used to find the
> +# memory usage for the GDB in this test.
> +# Returns memory usage in Kb, or -1 if it couldn't be found.
> +proc get_mem_usage {pid prefix} {
> + set fd [open "/proc/$pid/status"]
> + set memory -1
> + while {[gets $fd line] != -1} {
> + if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
> + set memory $mem
> + break
> + }
> + }
> + close $fd
> +
> + gdb_assert {$memory != -1} "$prefix: Managed to read the memory usage"
> +
> + return $memory
> +}
> +
> +# This proc restarts GDB, runs the inferior with the desired
> +# amount of memory, then checks how much memory is necessary
> +# to run the gcore command. It will return -1 if the gcore
> +# command fails, 0 otherwise.
> +proc run_test {megs} {
> + with_test_prefix "$megs Mb" {
> + clean_restart $::testfile
> +
> + set bin [standard_output_file $::testfile]
> + set corefile [standard_output_file "${::testfile}.core"]
> + set inferior_pid [eval exec $bin $megs &]
> + set gdb_pid [exp_pid -i [board_info host fileid]]
> +
> + # wait for memory allocation to finish.
> + sleep 1
> +
> + # Get the important info.
> + gdb_test "attach $inferior_pid" "Attaching to.*"
> + set mem_before [get_mem_usage $gdb_pid before]
> + if {![gdb_gcore_cmd $corefile "create the corefile"]} {
> + return -1
> + }
> + set mem_after [get_mem_usage $gdb_pid after]
> +
> + # Do the main part of the test: How much is the memory
> + # usage of GDB going to grow after using the gcore command.
> + set diff_k [expr $mem_after - $mem_before]
> + set diff [expr $diff_k/1024]
> + verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
> + # The original plan was to compare to a multiple of MEGS
> + # but since the requirements don't seem to go up as the
> + # inferior allocated more memory, we instead just hardcode
> + # 2 megs, since sometimes 1 is used.
> + gdb_assert {$diff < 2} \
> + "gdb did not use as much memory as the inferior"
> +
> + # Kill the inferior so we don't have to wait until the process
> + # finishes.
> + gdb_test "signal SIGKILL" ".*The program no longer exists."
> + }
> + return 0
> +}
> +
> +# If we couldn't create the first corefile, there's no point
> +# in running the second part of the test.
> +if {[run_test 4] != 0} {
> + return
> +}
> +# Surprisingly enough, the larger inferior doesn't seem to use
> +# any extra memory, it usually uses less memory. Which is good,
> +# it means our memory requirements aren't growing with the inferior.
> +run_test 64
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 15:24 ` Tom de Vries
@ 2025-02-24 19:47 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2025-02-24 19:47 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
On 2/24/25 12:24 PM, Tom de Vries wrote:
> On 2/24/25 15:04, Guinevere Larsen wrote:
>> For a long time, Fedora has been carrying an out-of-tree patch with a
>> similar test to the one proposed in this patch, that ensures that the
>> memory requirements don't grow with the inferior's memory. It's been
>> so long that the context for why this test exists has been lost, but
>> it looked like it could be interesting for upstream.
>>
>> The test runs twice, once with the inferior allocating 4Mb of memory,
>> and the other allocating 64Mb. My plan was to find the rate at which
>> things increase based on inferior size, and have that tested to ensure
>> we're not growing that requirement accidentally, but my testing
>> actually showed memory requirements going down as the inferior
>> increases,
>> so instead I just hardcoded that we need less than 2Mb for the command,
>> and it can be tweaked later if necessary.
>
> I tried out the test-case, and it passed for me, but after doing:
> ...
> $ echo "1" | sudo tee /proc/sys/kernel/yama/ptrace_scope
> ...
> I get:
> ...
> (gdb) attach 29408^M
> Attaching to program:
> /data/vries/gdb/leap-15-6/build/gdb/testsuite/outputs/gdb.base/gcore-memory-usage/gcore-memory-usage,
> process 29408^M
> ptrace: Operation not permitted.^M
> (gdb) PASS: gdb.base/gcore-memory-usage.exp: 4 Mb: attach 29408
> PASS: gdb.base/gcore-memory-usage.exp: 4 Mb: before: Managed to read
> the memory usage
> gcore
> /data/vries/gdb/leap-15-6/build/gdb/testsuite/outputs/gdb.base/gcore-memory-usage/gcore-memory-usage.core^M
> You can't do that without a process to debug.^M
> (gdb) FAIL: gdb.base/gcore-memory-usage.exp: 4 Mb: create the corefile
> ...
>
> So, this is missing "require can_spawn_for_attach".
Ah, thanks for spotting that. I'll fix this for v2 (that may take a
minute given Andrew's comments)...
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> - Tom
>
>> ---
>> gdb/testsuite/gdb.base/gcore-memory-usage.c | 37 +++++++
>> gdb/testsuite/gdb.base/gcore-memory-usage.exp | 97 +++++++++++++++++++
>> 2 files changed, 134 insertions(+)
>> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
>> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
>>
>> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c
>> b/gdb/testsuite/gdb.base/gcore-memory-usage.c
>> new file mode 100644
>> index 00000000000..514fdb905cf
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
>> @@ -0,0 +1,37 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2025 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/>. */
>> +
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +
>> +int main(int argc, char **argv)
>> +{
>> + /* Use argv to calculate how many megabytes to allocate.
>> + Do this to see how much gcore memory usage increases
>> + based on inferior dynamic memory. */
>> + int megs = atoi (argv[1]);
>> + char *p = malloc (megs * 1024 * 1024);
>> +
>> + /* Wait a long time so GDB can attach to this. */
>> + sleep (10);
>> +
>> + /* Let's be nice citizens :). */
>> + free (p);
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp
>> b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
>> new file mode 100644
>> index 00000000000..cdb6d73c11b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
>> @@ -0,0 +1,97 @@
>> +# Copyright (C) 2025 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/>.
>> +
>> +# Test that gcore doesn't end up using excessive memory.
>> +
>> +require {istarget "*-*-linux*"}
>> +
>> +standard_testfile
>> +
>> +if {[build_executable "failed to prepare" $testfile $srcfile
>> {debug}] == -1} {
>> + return -1
>> +}
>> +
>> +# Read the proc_pid_status page, to find how much memory the given
>> +# PID is using. This is meant to be used to find the
>> +# memory usage for the GDB in this test.
>> +# Returns memory usage in Kb, or -1 if it couldn't be found.
>> +proc get_mem_usage {pid prefix} {
>> + set fd [open "/proc/$pid/status"]
>> + set memory -1
>> + while {[gets $fd line] != -1} {
>> + if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
>> + set memory $mem
>> + break
>> + }
>> + }
>> + close $fd
>> +
>> + gdb_assert {$memory != -1} "$prefix: Managed to read the memory
>> usage"
>> +
>> + return $memory
>> +}
>> +
>> +# This proc restarts GDB, runs the inferior with the desired
>> +# amount of memory, then checks how much memory is necessary
>> +# to run the gcore command. It will return -1 if the gcore
>> +# command fails, 0 otherwise.
>> +proc run_test {megs} {
>> + with_test_prefix "$megs Mb" {
>> + clean_restart $::testfile
>> +
>> + set bin [standard_output_file $::testfile]
>> + set corefile [standard_output_file "${::testfile}.core"]
>> + set inferior_pid [eval exec $bin $megs &]
>> + set gdb_pid [exp_pid -i [board_info host fileid]]
>> +
>> + # wait for memory allocation to finish.
>> + sleep 1
>> +
>> + # Get the important info.
>> + gdb_test "attach $inferior_pid" "Attaching to.*"
>> + set mem_before [get_mem_usage $gdb_pid before]
>> + if {![gdb_gcore_cmd $corefile "create the corefile"]} {
>> + return -1
>> + }
>> + set mem_after [get_mem_usage $gdb_pid after]
>> +
>> + # Do the main part of the test: How much is the memory
>> + # usage of GDB going to grow after using the gcore command.
>> + set diff_k [expr $mem_after - $mem_before]
>> + set diff [expr $diff_k/1024]
>> + verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
>> + # The original plan was to compare to a multiple of MEGS
>> + # but since the requirements don't seem to go up as the
>> + # inferior allocated more memory, we instead just hardcode
>> + # 2 megs, since sometimes 1 is used.
>> + gdb_assert {$diff < 2} \
>> + "gdb did not use as much memory as the inferior"
>> +
>> + # Kill the inferior so we don't have to wait until the process
>> + # finishes.
>> + gdb_test "signal SIGKILL" ".*The program no longer exists."
>> + }
>> + return 0
>> +}
>> +
>> +# If we couldn't create the first corefile, there's no point
>> +# in running the second part of the test.
>> +if {[run_test 4] != 0} {
>> + return
>> +}
>> +# Surprisingly enough, the larger inferior doesn't seem to use
>> +# any extra memory, it usually uses less memory. Which is good,
>> +# it means our memory requirements aren't growing with the inferior.
>> +run_test 64
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 14:04 [PATCH] gdb/testsuite: add test for memory requirements of gcore Guinevere Larsen
2025-02-24 15:24 ` Tom de Vries
@ 2025-02-24 18:21 ` Andrew Burgess
2025-02-24 20:00 ` Guinevere Larsen
2025-02-25 13:45 ` [PATCH v2] " Guinevere Larsen
2 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2025-02-24 18:21 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Guinevere Larsen
Guinevere Larsen <guinevere@redhat.com> writes:
> For a long time, Fedora has been carrying an out-of-tree patch with a
> similar test to the one proposed in this patch, that ensures that the
> memory requirements don't grow with the inferior's memory. It's been
> so long that the context for why this test exists has been lost, but
> it looked like it could be interesting for upstream.
>
> The test runs twice, once with the inferior allocating 4Mb of memory,
> and the other allocating 64Mb. My plan was to find the rate at which
> things increase based on inferior size, and have that tested to ensure
> we're not growing that requirement accidentally, but my testing
> actually showed memory requirements going down as the inferior increases,
> so instead I just hardcoded that we need less than 2Mb for the command,
> and it can be tweaked later if necessary.
> ---
> gdb/testsuite/gdb.base/gcore-memory-usage.c | 37 +++++++
> gdb/testsuite/gdb.base/gcore-memory-usage.exp | 97 +++++++++++++++++++
> 2 files changed, 134 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
>
> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c b/gdb/testsuite/gdb.base/gcore-memory-usage.c
> new file mode 100644
> index 00000000000..514fdb905cf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
> @@ -0,0 +1,37 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2025 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/>. */
> +
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int main(int argc, char **argv)
This should use GNU style. So 'main' on a new line.
> +{
> + /* Use argv to calculate how many megabytes to allocate.
> + Do this to see how much gcore memory usage increases
> + based on inferior dynamic memory. */
> + int megs = atoi (argv[1]);
> + char *p = malloc (megs * 1024 * 1024);
> +
> + /* Wait a long time so GDB can attach to this. */
> + sleep (10);
Ideally we'd not use a hard coded sleep like this. Instead, you could
have the test set an alarm with some large delay (common for tests that
can spin), then have the test enter a spin loop:
while (some_flag_that_is_true)
sleep (1);
Then GDB can terminate the test by setting some_flag_that_is_true to
false if it wants, or you can just stick with the existing approach of
sending SIGKILL, which is fine too.
Adding an alarm call ensures the test will self terminate eventually, if
DejaGNU doesn't clean it up correctly for some reason.
> +
> + /* Let's be nice citizens :). */
> + free (p);
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
> new file mode 100644
> index 00000000000..cdb6d73c11b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
> @@ -0,0 +1,97 @@
> +# Copyright (C) 2025 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/>.
> +
> +# Test that gcore doesn't end up using excessive memory.
> +
> +require {istarget "*-*-linux*"}
> +
> +standard_testfile
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug}] == -1} {
> + return -1
> +}
> +
> +# Read the proc_pid_status page, to find how much memory the given
> +# PID is using. This is meant to be used to find the
> +# memory usage for the GDB in this test.
> +# Returns memory usage in Kb, or -1 if it couldn't be found.
> +proc get_mem_usage {pid prefix} {
> + set fd [open "/proc/$pid/status"]
> + set memory -1
> + while {[gets $fd line] != -1} {
> + if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
> + set memory $mem
> + break
> + }
> + }
> + close $fd
> +
> + gdb_assert {$memory != -1} "$prefix: Managed to read the memory usage"
> +
> + return $memory
> +}
> +
> +# This proc restarts GDB, runs the inferior with the desired
> +# amount of memory, then checks how much memory is necessary
> +# to run the gcore command. It will return -1 if the gcore
> +# command fails, 0 otherwise.
> +proc run_test {megs} {
> + with_test_prefix "$megs Mb" {
> + clean_restart $::testfile
> +
> + set bin [standard_output_file $::testfile]
Isn't bin the same as binfile, which is setup by standard_testfile? Or
am I missing something?
> + set corefile [standard_output_file "${::testfile}.core"]
> + set inferior_pid [eval exec $bin $megs &]
If there was no argument passing here then I'd say you should be using
spawn_wait_for_attach. But that doesn't support argument passing...
... however, if you read that proc (and its helper proc) you'll see some
comments that suggest using eval/exec like you do are not the right
choice.
So maybe we should either extend (somehow) spawn_wait_for_attach to
allow argument passing, or write something like spawn_wait_for_attach
that handles arguments?
> + set gdb_pid [exp_pid -i [board_info host fileid]]
> +
> + # wait for memory allocation to finish.
Capitalise comments please.
> + sleep 1
Ideally we'd not rely on sleeps like this. Better would be to attach,
and then ensure the inferior has reached some known point before sending
the gcore command.
Thanks,
Andrew
> +
> + # Get the important info.
> + gdb_test "attach $inferior_pid" "Attaching to.*"
> + set mem_before [get_mem_usage $gdb_pid before]
> + if {![gdb_gcore_cmd $corefile "create the corefile"]} {
> + return -1
> + }
> + set mem_after [get_mem_usage $gdb_pid after]
> +
> + # Do the main part of the test: How much is the memory
> + # usage of GDB going to grow after using the gcore command.
> + set diff_k [expr $mem_after - $mem_before]
> + set diff [expr $diff_k/1024]
> + verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
> + # The original plan was to compare to a multiple of MEGS
> + # but since the requirements don't seem to go up as the
> + # inferior allocated more memory, we instead just hardcode
> + # 2 megs, since sometimes 1 is used.
> + gdb_assert {$diff < 2} \
> + "gdb did not use as much memory as the inferior"
> +
> + # Kill the inferior so we don't have to wait until the process
> + # finishes.
> + gdb_test "signal SIGKILL" ".*The program no longer exists."
> + }
> + return 0
> +}
> +
> +# If we couldn't create the first corefile, there's no point
> +# in running the second part of the test.
> +if {[run_test 4] != 0} {
> + return
> +}
> +# Surprisingly enough, the larger inferior doesn't seem to use
> +# any extra memory, it usually uses less memory. Which is good,
> +# it means our memory requirements aren't growing with the inferior.
> +run_test 64
> --
> 2.48.1
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 18:21 ` Andrew Burgess
@ 2025-02-24 20:00 ` Guinevere Larsen
2025-02-24 20:18 ` Guinevere Larsen
0 siblings, 1 reply; 14+ messages in thread
From: Guinevere Larsen @ 2025-02-24 20:00 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2/24/25 3:21 PM, Andrew Burgess wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> For a long time, Fedora has been carrying an out-of-tree patch with a
>> similar test to the one proposed in this patch, that ensures that the
>> memory requirements don't grow with the inferior's memory. It's been
>> so long that the context for why this test exists has been lost, but
>> it looked like it could be interesting for upstream.
>>
>> The test runs twice, once with the inferior allocating 4Mb of memory,
>> and the other allocating 64Mb. My plan was to find the rate at which
>> things increase based on inferior size, and have that tested to ensure
>> we're not growing that requirement accidentally, but my testing
>> actually showed memory requirements going down as the inferior increases,
>> so instead I just hardcoded that we need less than 2Mb for the command,
>> and it can be tweaked later if necessary.
>> ---
>> gdb/testsuite/gdb.base/gcore-memory-usage.c | 37 +++++++
>> gdb/testsuite/gdb.base/gcore-memory-usage.exp | 97 +++++++++++++++++++
>> 2 files changed, 134 insertions(+)
>> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
>> create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
>>
>> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c b/gdb/testsuite/gdb.base/gcore-memory-usage.c
>> new file mode 100644
>> index 00000000000..514fdb905cf
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
>> @@ -0,0 +1,37 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2025 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/>. */
>> +
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +
>> +int main(int argc, char **argv)
> This should use GNU style. So 'main' on a new line.
oops, sorry, habits...
>
>> +{
>> + /* Use argv to calculate how many megabytes to allocate.
>> + Do this to see how much gcore memory usage increases
>> + based on inferior dynamic memory. */
>> + int megs = atoi (argv[1]);
>> + char *p = malloc (megs * 1024 * 1024);
>> +
>> + /* Wait a long time so GDB can attach to this. */
>> + sleep (10);
> Ideally we'd not use a hard coded sleep like this. Instead, you could
> have the test set an alarm with some large delay (common for tests that
> can spin), then have the test enter a spin loop:
>
> while (some_flag_that_is_true)
> sleep (1);
>
> Then GDB can terminate the test by setting some_flag_that_is_true to
> false if it wants, or you can just stick with the existing approach of
> sending SIGKILL, which is fine too.
>
> Adding an alarm call ensures the test will self terminate eventually, if
> DejaGNU doesn't clean it up correctly for some reason.
Oh, I see. This makes sense, I'll go with it then
>
>> +
>> + /* Let's be nice citizens :). */
>> + free (p);
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
>> new file mode 100644
>> index 00000000000..cdb6d73c11b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
>> @@ -0,0 +1,97 @@
>> +# Copyright (C) 2025 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/>.
>> +
>> +# Test that gcore doesn't end up using excessive memory.
>> +
>> +require {istarget "*-*-linux*"}
>> +
>> +standard_testfile
>> +
>> +if {[build_executable "failed to prepare" $testfile $srcfile {debug}] == -1} {
>> + return -1
>> +}
>> +
>> +# Read the proc_pid_status page, to find how much memory the given
>> +# PID is using. This is meant to be used to find the
>> +# memory usage for the GDB in this test.
>> +# Returns memory usage in Kb, or -1 if it couldn't be found.
>> +proc get_mem_usage {pid prefix} {
>> + set fd [open "/proc/$pid/status"]
>> + set memory -1
>> + while {[gets $fd line] != -1} {
>> + if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
>> + set memory $mem
>> + break
>> + }
>> + }
>> + close $fd
>> +
>> + gdb_assert {$memory != -1} "$prefix: Managed to read the memory usage"
>> +
>> + return $memory
>> +}
>> +
>> +# This proc restarts GDB, runs the inferior with the desired
>> +# amount of memory, then checks how much memory is necessary
>> +# to run the gcore command. It will return -1 if the gcore
>> +# command fails, 0 otherwise.
>> +proc run_test {megs} {
>> + with_test_prefix "$megs Mb" {
>> + clean_restart $::testfile
>> +
>> + set bin [standard_output_file $::testfile]
> Isn't bin the same as binfile, which is setup by standard_testfile? Or
> am I missing something?
I think I forgot about the existence of binfile. This was copied from
the upstream test...
>
>> + set corefile [standard_output_file "${::testfile}.core"]
>> + set inferior_pid [eval exec $bin $megs &]
> If there was no argument passing here then I'd say you should be using
> spawn_wait_for_attach. But that doesn't support argument passing...
>
> ... however, if you read that proc (and its helper proc) you'll see some
> comments that suggest using eval/exec like you do are not the right
> choice.
>
> So maybe we should either extend (somehow) spawn_wait_for_attach to
> allow argument passing, or write something like spawn_wait_for_attach
> that handles arguments?
If I found the correct documentation page, I think it shouldn't be hard
to extend spawn_wait_for_attach to handle arguments. And using optional
arguments, it should be a very minor patch, so I'll see what I can cook up.
Thanks for the pointer, this definitely sounds like a better solution.
>
>> + set gdb_pid [exp_pid -i [board_info host fileid]]
>> +
>> + # wait for memory allocation to finish.
> Capitalise comments please.
>
>> + sleep 1
> Ideally we'd not rely on sleeps like this. Better would be to attach,
> and then ensure the inferior has reached some known point before sending
> the gcore command.
Right, I see. This makes sense if the inferior can spin, so I'll switch
to this.
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> Andrew
>
>> +
>> + # Get the important info.
>> + gdb_test "attach $inferior_pid" "Attaching to.*"
>> + set mem_before [get_mem_usage $gdb_pid before]
>> + if {![gdb_gcore_cmd $corefile "create the corefile"]} {
>> + return -1
>> + }
>> + set mem_after [get_mem_usage $gdb_pid after]
>> +
>> + # Do the main part of the test: How much is the memory
>> + # usage of GDB going to grow after using the gcore command.
>> + set diff_k [expr $mem_after - $mem_before]
>> + set diff [expr $diff_k/1024]
>> + verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
>> + # The original plan was to compare to a multiple of MEGS
>> + # but since the requirements don't seem to go up as the
>> + # inferior allocated more memory, we instead just hardcode
>> + # 2 megs, since sometimes 1 is used.
>> + gdb_assert {$diff < 2} \
>> + "gdb did not use as much memory as the inferior"
>> +
>> + # Kill the inferior so we don't have to wait until the process
>> + # finishes.
>> + gdb_test "signal SIGKILL" ".*The program no longer exists."
>> + }
>> + return 0
>> +}
>> +
>> +# If we couldn't create the first corefile, there's no point
>> +# in running the second part of the test.
>> +if {[run_test 4] != 0} {
>> + return
>> +}
>> +# Surprisingly enough, the larger inferior doesn't seem to use
>> +# any extra memory, it usually uses less memory. Which is good,
>> +# it means our memory requirements aren't growing with the inferior.
>> +run_test 64
>> --
>> 2.48.1
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 20:00 ` Guinevere Larsen
@ 2025-02-24 20:18 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2025-02-24 20:18 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]
On 2/24/25 5:00 PM, Guinevere Larsen wrote:
>> If there was no argument passing here then I'd say you should be using
>> spawn_wait_for_attach. But that doesn't support argument passing...
>>
>> ... however, if you read that proc (and its helper proc) you'll see some
>> comments that suggest using eval/exec like you do are not the right
>> choice.
>>
>> So maybe we should either extend (somehow) spawn_wait_for_attach to
>> allow argument passing, or write something like spawn_wait_for_attach
>> that handles arguments?
>
> If I found the correct documentation page, I think it shouldn't be
> hard to extend spawn_wait_for_attach to handle arguments. And using
> optional arguments, it should be a very minor patch, so I'll see what
> I can cook up.
>
> Thanks for the pointer, this definitely sounds like a better solution.
Actually, scratch that, we don't *need* to extend spawn_wait_for_attach.
We can just give it a list with one element: "$::binfile $megs". So I'll
definitely go this route.
I can add a convenience function to handle this anyway, if you'd prefer,
though
--
Cheers,
Guinevere Larsen
She/Her/Hers
[-- Attachment #2: Type: text/html, Size: 1874 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] gdb/testsuite: add test for memory requirements of gcore
2025-02-24 14:04 [PATCH] gdb/testsuite: add test for memory requirements of gcore Guinevere Larsen
2025-02-24 15:24 ` Tom de Vries
2025-02-24 18:21 ` Andrew Burgess
@ 2025-02-25 13:45 ` Guinevere Larsen
2025-02-26 17:50 ` [PATCH v3] " Guinevere Larsen
2 siblings, 1 reply; 14+ messages in thread
From: Guinevere Larsen @ 2025-02-25 13:45 UTC (permalink / raw)
To: gdb-patches; +Cc: Guinevere Larsen
For a long time, Fedora has been carrying an out-of-tree patch with a
similar test to the one proposed in this patch, that ensures that the
memory requirements don't grow with the inferior's memory. It's been
so long that the context for why this test exists has been lost, but
it looked like it could be interesting for upstream.
The test runs twice, once with the inferior allocating 4Mb of memory,
and the other allocating 64Mb. My plan was to find the rate at which
things increase based on inferior size, and have that tested to ensure
we're not growing that requirement accidentally, but my testing
actually showed memory requirements going down as the inferior increases,
so instead I just hardcoded that we need less than 2Mb for the command,
and it can be tweaked later if necessary.
---
gdb/testsuite/gdb.base/gcore-memory-usage.c | 57 +++++++++++
gdb/testsuite/gdb.base/gcore-memory-usage.exp | 96 +++++++++++++++++++
2 files changed, 153 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c b/gdb/testsuite/gdb.base/gcore-memory-usage.c
new file mode 100644
index 00000000000..4530482abbf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
@@ -0,0 +1,57 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2025 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/>. */
+
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int spin = 1;
+
+int
+main (int argc, char **argv)
+{
+ int i = 0;
+ /* Small quality of life thing for people re-running tests
+ manually. */
+ if (argc < 2)
+ {
+ printf("Usage: %s [Megs]", argv[0]);
+ return 1;
+ }
+ /* Use argv to calculate how many megabytes to allocate.
+ Do this to see how much gcore memory usage increases
+ based on inferior dynamic memory. */
+ int megs = atoi (argv[1]);
+ char *p = malloc (megs * 1024 * 1024);
+
+ /* Setup an alarm, in case something goes wrong with testing. */
+ alarm (300);
+
+ /* Wait a long time so GDB can attach to this.
+ We need to have the second statement inside of the loop
+ to sidestep PR breakpoints/32744. */
+ while (spin)
+ {
+ sleep (1);
+ i++; /* TAG: BREAK HERE */
+ }
+
+ /* Let's be nice citizens :). */
+ free (p);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
new file mode 100644
index 00000000000..1e31578d4a9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
@@ -0,0 +1,96 @@
+# Copyright (C) 2025 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/>.
+
+# Test that gcore doesn't end up using excessive memory.
+
+require {istarget "*-*-linux*"}
+require can_spawn_for_attach
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile {debug}] == -1} {
+ return -1
+}
+
+# Read the proc_pid_status page, to find how much memory the given
+# PID is using. This is meant to be used to find the
+# memory usage for the GDB in this test.
+# Returns memory usage in Kb, or -1 if it couldn't be found.
+proc get_mem_usage {pid prefix} {
+ set fd [open "/proc/$pid/status"]
+ set memory -1
+ while {[gets $fd line] != -1} {
+ if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
+ set memory $mem
+ break
+ }
+ }
+ close $fd
+
+ gdb_assert {$memory != -1} "$prefix: Managed to read the memory usage"
+
+ return $memory
+}
+
+# This proc restarts GDB, runs the inferior with the desired
+# amount of memory, then checks how much memory is necessary
+# to run the gcore command. It will return -1 if the gcore
+# command fails, 0 otherwise.
+proc run_test {megs} {
+ with_test_prefix "$megs Mb" {
+ clean_restart $::testfile
+
+ set corefile [standard_output_file "${::testfile}.core"]
+ set inferior_spawn_id [spawn_wait_for_attach [list "$::binfile $megs"]]
+ set inferior_pid [spawn_id_get_pid $inferior_spawn_id]
+ set gdb_pid [exp_pid -i [board_info host fileid]]
+
+ gdb_test "attach $inferior_pid" "Attaching to.*"
+ set line [gdb_get_line_number "TAG: BREAK HERE" $::testfile.c]
+ gdb_breakpoint "$line" "break at to known line"
+ gdb_continue_to_breakpoint "continue to known line"
+
+ # Get the important info.
+ set mem_before [get_mem_usage $gdb_pid before]
+ if {![gdb_gcore_cmd $corefile "create the corefile"]} {
+ return -1
+ }
+ set mem_after [get_mem_usage $gdb_pid after]
+
+ # Do the main part of the test: How much is the memory
+ # usage of GDB going to grow after using the gcore command.
+ set diff_k [expr $mem_after - $mem_before]
+ set diff [expr $diff_k/1024]
+ verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
+ # The original plan was to compare to a multiple of MEGS
+ # but since the requirements don't seem to go up as the
+ # inferior allocated more memory, we instead just hardcode
+ # 2 megs, since sometimes 1 is used.
+ gdb_assert {$diff < 2} "gdb did not use too much memory"
+
+ gdb_test_no_output "set spin=0" "Allow program to exit"
+ }
+ return 0
+}
+
+# If we couldn't create the first corefile, there's no point
+# in running the second part of the test.
+if {[run_test 4] != 0} {
+ return
+}
+# Surprisingly enough, the larger inferior doesn't seem to use
+# any extra memory, it usually uses less memory. Which is good,
+# it means our memory requirements aren't growing with the inferior.
+run_test 64
--
2.48.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-02-25 13:45 ` [PATCH v2] " Guinevere Larsen
@ 2025-02-26 17:50 ` Guinevere Larsen
2025-03-04 18:19 ` Tom Tromey
2025-03-11 16:14 ` Simon Marchi
0 siblings, 2 replies; 14+ messages in thread
From: Guinevere Larsen @ 2025-02-26 17:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Guinevere Larsen
For a long time, Fedora has been carrying an out-of-tree patch with a
similar test to the one proposed in this patch, that ensures that the
memory requirements don't grow with the inferior's memory. It's been
so long that the context for why this test exists has been lost, but
it looked like it could be interesting for upstream.
The test runs twice, once with the inferior allocating 4Mb of memory,
and the other allocating 64Mb. My plan was to find the rate at which
things increase based on inferior size, and have that tested to ensure
we're not growing that requirement accidentally, but my testing
actually showed memory requirements going down as the inferior increases,
so instead I just hardcoded that we need less than 2Mb for the command,
and it can be tweaked later if necessary.
---
Linaro flagged an issue in v2, where GDB couldn't set a breakpoint in
line 51 (where i was incremented). That sounds like a gcc issue,
optimizing i away because it was unused, but to avoid having this come
and go as GCC changes, I changed to set a bp directly on the sleep line.
This should work now.
---
gdb/testsuite/gdb.base/gcore-memory-usage.c | 53 ++++++++++
gdb/testsuite/gdb.base/gcore-memory-usage.exp | 96 +++++++++++++++++++
2 files changed, 149 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.c
create mode 100644 gdb/testsuite/gdb.base/gcore-memory-usage.exp
diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.c b/gdb/testsuite/gdb.base/gcore-memory-usage.c
new file mode 100644
index 00000000000..a4a197243a2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-memory-usage.c
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2025 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/>. */
+
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int spin = 1;
+
+int
+main (int argc, char **argv)
+{
+ /* Small quality of life thing for people re-running tests
+ manually. */
+ if (argc < 2)
+ {
+ printf("Usage: %s [Megs]", argv[0]);
+ return 1;
+ }
+ /* Use argv to calculate how many megabytes to allocate.
+ Do this to see how much gcore memory usage increases
+ based on inferior dynamic memory. */
+ int megs = atoi (argv[1]);
+ char *p = malloc (megs * 1024 * 1024);
+
+ /* Setup an alarm, in case something goes wrong with testing. */
+ alarm (300);
+
+ /* Wait a long time so GDB can attach to this.
+ We need to have the second statement inside of the loop
+ to sidestep PR breakpoints/32744. */
+ while (spin)
+ sleep (1);/* TAG: BREAK HERE */
+
+ /* Let's be nice citizens :). */
+ free (p);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
new file mode 100644
index 00000000000..1e31578d4a9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
@@ -0,0 +1,96 @@
+# Copyright (C) 2025 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/>.
+
+# Test that gcore doesn't end up using excessive memory.
+
+require {istarget "*-*-linux*"}
+require can_spawn_for_attach
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile {debug}] == -1} {
+ return -1
+}
+
+# Read the proc_pid_status page, to find how much memory the given
+# PID is using. This is meant to be used to find the
+# memory usage for the GDB in this test.
+# Returns memory usage in Kb, or -1 if it couldn't be found.
+proc get_mem_usage {pid prefix} {
+ set fd [open "/proc/$pid/status"]
+ set memory -1
+ while {[gets $fd line] != -1} {
+ if {[regexp {VmSize:\s*([0-9]+) kB} $line full mem]} {
+ set memory $mem
+ break
+ }
+ }
+ close $fd
+
+ gdb_assert {$memory != -1} "$prefix: Managed to read the memory usage"
+
+ return $memory
+}
+
+# This proc restarts GDB, runs the inferior with the desired
+# amount of memory, then checks how much memory is necessary
+# to run the gcore command. It will return -1 if the gcore
+# command fails, 0 otherwise.
+proc run_test {megs} {
+ with_test_prefix "$megs Mb" {
+ clean_restart $::testfile
+
+ set corefile [standard_output_file "${::testfile}.core"]
+ set inferior_spawn_id [spawn_wait_for_attach [list "$::binfile $megs"]]
+ set inferior_pid [spawn_id_get_pid $inferior_spawn_id]
+ set gdb_pid [exp_pid -i [board_info host fileid]]
+
+ gdb_test "attach $inferior_pid" "Attaching to.*"
+ set line [gdb_get_line_number "TAG: BREAK HERE" $::testfile.c]
+ gdb_breakpoint "$line" "break at to known line"
+ gdb_continue_to_breakpoint "continue to known line"
+
+ # Get the important info.
+ set mem_before [get_mem_usage $gdb_pid before]
+ if {![gdb_gcore_cmd $corefile "create the corefile"]} {
+ return -1
+ }
+ set mem_after [get_mem_usage $gdb_pid after]
+
+ # Do the main part of the test: How much is the memory
+ # usage of GDB going to grow after using the gcore command.
+ set diff_k [expr $mem_after - $mem_before]
+ set diff [expr $diff_k/1024]
+ verbose -log "The gcore command used $diff Mb ($diff_k Kb)"
+ # The original plan was to compare to a multiple of MEGS
+ # but since the requirements don't seem to go up as the
+ # inferior allocated more memory, we instead just hardcode
+ # 2 megs, since sometimes 1 is used.
+ gdb_assert {$diff < 2} "gdb did not use too much memory"
+
+ gdb_test_no_output "set spin=0" "Allow program to exit"
+ }
+ return 0
+}
+
+# If we couldn't create the first corefile, there's no point
+# in running the second part of the test.
+if {[run_test 4] != 0} {
+ return
+}
+# Surprisingly enough, the larger inferior doesn't seem to use
+# any extra memory, it usually uses less memory. Which is good,
+# it means our memory requirements aren't growing with the inferior.
+run_test 64
--
2.48.1
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-02-26 17:50 ` [PATCH v3] " Guinevere Larsen
@ 2025-03-04 18:19 ` Tom Tromey
2025-03-05 20:03 ` Guinevere Larsen
2025-03-11 16:14 ` Simon Marchi
1 sibling, 1 reply; 14+ messages in thread
From: Tom Tromey @ 2025-03-04 18:19 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
Guinevere> The test runs twice, once with the inferior allocating 4Mb of memory,
Guinevere> and the other allocating 64Mb. My plan was to find the rate at which
Guinevere> things increase based on inferior size, and have that tested to ensure
Guinevere> we're not growing that requirement accidentally, but my testing
Guinevere> actually showed memory requirements going down as the inferior increases,
Guinevere> so instead I just hardcoded that we need less than 2Mb for the command,
Guinevere> and it can be tweaked later if necessary.
This seems fine to me. I didn't understand why it needed to attach, it
seems like just running it would be the same? But also it doesn't
really matter, considering you've done the work.
Approved-By: Tom Tromey <tom@tromey.com>
thanks,
Tom
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-03-04 18:19 ` Tom Tromey
@ 2025-03-05 20:03 ` Guinevere Larsen
2025-03-05 20:21 ` Tom Tromey
0 siblings, 1 reply; 14+ messages in thread
From: Guinevere Larsen @ 2025-03-05 20:03 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 3/4/25 3:19 PM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> The test runs twice, once with the inferior allocating 4Mb of memory,
> Guinevere> and the other allocating 64Mb. My plan was to find the rate at which
> Guinevere> things increase based on inferior size, and have that tested to ensure
> Guinevere> we're not growing that requirement accidentally, but my testing
> Guinevere> actually showed memory requirements going down as the inferior increases,
> Guinevere> so instead I just hardcoded that we need less than 2Mb for the command,
> Guinevere> and it can be tweaked later if necessary.
>
> This seems fine to me. I didn't understand why it needed to attach, it
> seems like just running it would be the same? But also it doesn't
> really matter, considering you've done the work.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> thanks,
> Tom
>
I noticed today as I returned from a holiday that I misunderstood the
issue on v2, so I made this small change to the test, to stop it from
falling. I'll push the test on friday if there are no comments since I
think it is a pretty trivial change.
---
diff --git a/gdb/testsuite/gdb.base/gcore-memory-usage.exp
b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
index 1e31578d4a9..db7f270a5e8 100644
--- a/gdb/testsuite/gdb.base/gcore-memory-usage.exp
+++ b/gdb/testsuite/gdb.base/gcore-memory-usage.exp
@@ -59,7 +59,7 @@ proc run_test {megs} {
gdb_test "attach $inferior_pid" "Attaching to.*"
set line [gdb_get_line_number "TAG: BREAK HERE" $::testfile.c]
- gdb_breakpoint "$line" "break at to known line"
+ gdb_breakpoint "${::srcfile}:$line" "break at to known line"
gdb_continue_to_breakpoint "continue to known line"
# Get the important info.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-03-05 20:03 ` Guinevere Larsen
@ 2025-03-05 20:21 ` Tom Tromey
2025-03-05 20:39 ` Guinevere Larsen
0 siblings, 1 reply; 14+ messages in thread
From: Tom Tromey @ 2025-03-05 20:21 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: Tom Tromey, gdb-patches
>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
Guinevere> I noticed today as I returned from a holiday that I misunderstood the
Guinevere> issue on v2, so I made this small change to the test, to stop it from
Guinevere> falling. I'll push the test on friday if there are no comments since I
Guinevere> think it is a pretty trivial change.
Yeah, you should just go ahead, thanks.
Tom
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-03-05 20:21 ` Tom Tromey
@ 2025-03-05 20:39 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2025-03-05 20:39 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 3/5/25 5:21 PM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> I noticed today as I returned from a holiday that I misunderstood the
> Guinevere> issue on v2, so I made this small change to the test, to stop it from
> Guinevere> falling. I'll push the test on friday if there are no comments since I
> Guinevere> think it is a pretty trivial change.
>
> Yeah, you should just go ahead, thanks.
>
> Tom
>
alright, pushed with the change, thank you!
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-02-26 17:50 ` [PATCH v3] " Guinevere Larsen
2025-03-04 18:19 ` Tom Tromey
@ 2025-03-11 16:14 ` Simon Marchi
2025-03-11 16:43 ` Guinevere Larsen
1 sibling, 1 reply; 14+ messages in thread
From: Simon Marchi @ 2025-03-11 16:14 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
On 2/26/25 12:50 PM, Guinevere Larsen wrote:
> For a long time, Fedora has been carrying an out-of-tree patch with a
> similar test to the one proposed in this patch, that ensures that the
> memory requirements don't grow with the inferior's memory. It's been
> so long that the context for why this test exists has been lost, but
> it looked like it could be interesting for upstream.
>
> The test runs twice, once with the inferior allocating 4Mb of memory,
> and the other allocating 64Mb. My plan was to find the rate at which
> things increase based on inferior size, and have that tested to ensure
> we're not growing that requirement accidentally, but my testing
> actually showed memory requirements going down as the inferior increases,
> so instead I just hardcoded that we need less than 2Mb for the command,
> and it can be tweaked later if necessary.
> ---
>
> Linaro flagged an issue in v2, where GDB couldn't set a breakpoint in
> line 51 (where i was incremented). That sounds like a gcc issue,
> optimizing i away because it was unused, but to avoid having this come
> and go as GCC changes, I changed to set a bp directly on the sleep line.
> This should work now.
On my CI, I see:
The gcore command used 2 Mb (3004 Kb)
FAIL: gdb.base/gcore-memory-usage.exp: 4 Mb: gdb did not use too much memory
The gcore command used 2 Mb (3004 Kb)
FAIL: gdb.base/gcore-memory-usage.exp: 64 Mb: gdb did not use too much memory
Locally if I try to build with the same configuration, I get:
The gcore command used 1 Mb (1640 Kb)
PASS: gdb.base/gcore-memory-usage.exp: 64 Mb: gdb did not use too much memory
I don't know why there's a difference. It's not the same distribution
or tool versions, it could be a lot of things.
Note that I build with Asan, UBSan and -D_GLIBCXX_DEBUG=1, all of which
can raise the memory usage. Also, on the CI, the workers are
containers, so perhaps it does something to the reported stats.
I don't really know if I should investigate that further or just bump
the threshold for "too much memory".
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] gdb/testsuite: add test for memory requirements of gcore
2025-03-11 16:14 ` Simon Marchi
@ 2025-03-11 16:43 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2025-03-11 16:43 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 3/11/25 1:14 PM, Simon Marchi wrote:
> On 2/26/25 12:50 PM, Guinevere Larsen wrote:
>> For a long time, Fedora has been carrying an out-of-tree patch with a
>> similar test to the one proposed in this patch, that ensures that the
>> memory requirements don't grow with the inferior's memory. It's been
>> so long that the context for why this test exists has been lost, but
>> it looked like it could be interesting for upstream.
>>
>> The test runs twice, once with the inferior allocating 4Mb of memory,
>> and the other allocating 64Mb. My plan was to find the rate at which
>> things increase based on inferior size, and have that tested to ensure
>> we're not growing that requirement accidentally, but my testing
>> actually showed memory requirements going down as the inferior increases,
>> so instead I just hardcoded that we need less than 2Mb for the command,
>> and it can be tweaked later if necessary.
>> ---
>>
>> Linaro flagged an issue in v2, where GDB couldn't set a breakpoint in
>> line 51 (where i was incremented). That sounds like a gcc issue,
>> optimizing i away because it was unused, but to avoid having this come
>> and go as GCC changes, I changed to set a bp directly on the sleep line.
>> This should work now.
> On my CI, I see:
>
> The gcore command used 2 Mb (3004 Kb)
> FAIL: gdb.base/gcore-memory-usage.exp: 4 Mb: gdb did not use too much memory
>
> The gcore command used 2 Mb (3004 Kb)
> FAIL: gdb.base/gcore-memory-usage.exp: 64 Mb: gdb did not use too much memory
>
> Locally if I try to build with the same configuration, I get:
>
> The gcore command used 1 Mb (1640 Kb)
> PASS: gdb.base/gcore-memory-usage.exp: 64 Mb: gdb did not use too much memory
>
> I don't know why there's a difference. It's not the same distribution
> or tool versions, it could be a lot of things.
>
> Note that I build with Asan, UBSan and -D_GLIBCXX_DEBUG=1, all of which
> can raise the memory usage. Also, on the CI, the workers are
> containers, so perhaps it does something to the reported stats.
>
> I don't really know if I should investigate that further or just bump
> the threshold for "too much memory".
>
> Simon
>
The original test just hardcoded 4 Mbs of memory for the 64Mb case (and
only had that case). I tried to make it more useful by analyzing the
growth of memory usage instead, but there seems to be no growth in
memory... So I think it's fine to just bump it up.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 14+ messages in thread