* [PATCH] Teach GDB to generate sparse core files (PR corefiles/31494)
@ 2024-03-15 18:27 Pedro Alves
2024-03-15 18:40 ` Eli Zaretskii
2024-03-18 13:29 ` [PATCH v2] " Pedro Alves
0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2024-03-15 18:27 UTC (permalink / raw)
To: gdb-patches; +Cc: Lancelot Six
This commit teaches GDB's gcore command to generate sparse core files
(if supported by the filesystem).
To create a sparse file, all you have to do is skip writing zeros to
the file, instead lseek'ing-ahead over them.
The sparse logic is applied when writing the memory sections, as
that's where the bulk of the data and the zeros are.
The commit also tweaks gdb.base/bigcore.exp to make it exercise
gdb-generated cores in addition to kernel-generated cores. We
couldn't do that before, because GDB's gcore on that test's program
would generate a multi-GB non-sparse core (16GB on my system).
After this commit, gdb.base/bigcore.exp generates, when testing with
GDB's gcore, a much smaller core file, roughly in line with what the
kernel produces:
real sizes:
$ du --hu testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
2.2M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
2.0M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
apparent sizes:
$ du --hu --apparent-size testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
Time to generate the core also goes down significantly. On my machine, I get:
when writing to an SSD, from 21.0s, down to 8.0s
when writing to an HDD, from 31.0s, down to 8.5s
The changes to gdb.base/bigcore.exp are smaller than they look at
first sight. It's basically mostly refactoring -- moving most of the
code to a new procedure which takes as argument who should dump the
core, and then calling the procedure twice. I purposedly did not
modernize any of the refactored code in this patch.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
Reviewed-By: Lancelot Six <lancelot.six@amd.com>
Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
---
gdb/NEWS | 4 +
gdb/doc/gdb.texinfo | 3 +
gdb/gcore.c | 115 +++++++++++++-
gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
4 files changed, 252 insertions(+), 108 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index d8ac0bb06a7..d1d25e4c24d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,10 @@ disassemble
command will now give an error. Previously the 'b' flag would
always override the 'r' flag.
+gcore
+generate-core-file
+ GDB now generates sparse core files, on systems that support it.
+
maintenance info line-table
Add an EPILOGUE-BEGIN column to the output of the command. It indicates
if the line is considered the start of the epilgoue, and thus a point at
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f093ee269e2..9224829bd93 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -13867,6 +13867,9 @@ Produce a core dump of the inferior process. The optional argument
specified, the file name defaults to @file{core.@var{pid}}, where
@var{pid} is the inferior process ID.
+If supported by the filesystem where the core is written to,
+@value{GDBN} generates a sparse core dump file.
+
Note that this command is implemented only for some systems (as of
this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 7c12aa3a777..8d1aafce8d9 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -39,10 +39,18 @@
#include "gdbsupport/byte-vector.h"
#include "gdbsupport/scope-exit.h"
+/* To generate sparse cores, we look at the data to write in chunks of
+ this size when considering whether to skip the write. Only if we
+ have a full block of this size with all zeros do we skip writing
+ it. A simpler algorithm that would try to skip all zeros would
+ result in potentially many more write/lseek syscalls, as normal
+ data is typically sprinkled with many small holes of zeros. */
+#define SPARSE_BLOCK_SIZE 0x1000
+
/* The largest amount of memory to read from the target at once. We
must throttle it to limit the amount of memory used by GDB during
generate-core-file for programs with large resident data. */
-#define MAX_COPY_BYTES (1024 * 1024)
+#define MAX_COPY_BYTES (256 * SPARSE_BLOCK_SIZE)
static const char *default_gcore_target (void);
static enum bfd_architecture default_gcore_arch (void);
@@ -98,7 +106,12 @@ write_gcore_file_1 (bfd *obfd)
bfd_set_section_alignment (note_sec, 0);
bfd_set_section_size (note_sec, note_size);
- /* Now create the memory/load sections. */
+ /* Now create the memory/load sections. Note
+ gcore_memory_sections's sparse logic is assuming that we'll
+ always write something afterwards, which we do: just below, we
+ write the note section. So there's no need for an ftruncate-like
+ call to grow the file to the right size if the last memory
+ sections were zeros and we skipped writing them. */
if (gcore_memory_sections (obfd) == 0)
error (_("gcore: failed to get corefile memory sections from target."));
@@ -567,6 +580,99 @@ objfile_find_memory_regions (struct target_ops *self,
return 0;
}
+/* Return true if we have a block full of zeros at DATA within the
+ [DATA, DATA+SIZE) buffer, false otherwise. */
+
+static bool
+is_all_zero_block (const gdb_byte *data, size_t size)
+{
+ if (size < SPARSE_BLOCK_SIZE)
+ return false;
+
+ /* A memcmp of a whole block is much faster than a simple for loop.
+ This makes a big difference, as with a for loop, this code would
+ dominate the performance and result in doubling the time to
+ generate a core, at the time of writing. With an optimized
+ memcmp, this doesn't even show up in the perf trace. */
+ static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
+ return memcmp (data, all_zero_block, SPARSE_BLOCK_SIZE) == 0;
+}
+
+/* Find the next all-zero block at DATA+OFFSET within the [DATA,
+ DATA+SIZE) buffer. Returns the offset to the all-zero block if
+ found, or zero if not found. */
+
+static size_t
+next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
+{
+ for (; offset < size; offset += SPARSE_BLOCK_SIZE)
+ if (is_all_zero_block (data + offset, size - offset))
+ return offset;
+ return 0;
+}
+
+/* Wrapper around bfd_set_section_contents that avoids writing
+ all-zero blocks to disk, so we create a sparse core file. */
+
+static bool
+sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
+ const gdb_byte *data,
+ size_t sec_offset,
+ size_t size)
+{
+ /* Note, we don't have to have special handling for the case of the
+ last memory region ending with zeros, because our caller always
+ writes out the note section after the memory/load sections. If
+ it didn't, we'd have to seek+write the last byte to make the file
+ size correct. (Or add an ftruncate abstraction to bfd and call
+ that.)
+
+ If the blocks we skip are not aligned with the filesystem blocks,
+ on filesystems with fixed blocked size, we may waste a tiny bit
+ of file size, as the blocks that are adjacent to all-zero blocks
+ will have to include an amount of zeros. However, in practice,
+ sections have some alignment, and so SEC_OFFSET will be aligned
+ too, as our caller reads-in memory in chunks of SPARSE_BLOCK_SIZE
+ multiples. It's just not worth the bother to worry about
+ alignment here. */
+ size_t data_offset = 0;
+ while (data_offset < size)
+ {
+ if (is_all_zero_block (data + data_offset, size - data_offset))
+ data_offset += SPARSE_BLOCK_SIZE;
+ else
+ {
+ /* We have some non-zero data to write to file. Find the
+ next all-zero block within the data, and only write up to
+ it. */
+ size_t next_all_zero_block_offset
+ = next_all_zero_block (data, data_offset + SPARSE_BLOCK_SIZE, size);
+ size_t next_data_offset = (next_all_zero_block_offset == 0
+ ? size
+ : next_all_zero_block_offset);
+
+ if (!bfd_set_section_contents (obfd, osec, data + data_offset,
+ sec_offset + data_offset,
+ next_data_offset - data_offset))
+ {
+ warning (_("Failed to write corefile contents (%s)."),
+ bfd_errmsg (bfd_get_error ()));
+ return false;
+ }
+
+ data_offset = next_data_offset;
+
+ /* If we already know we have an all-zero block at the next
+ offset, we can skip calling is_all_zero_block for it
+ again. */
+ if (next_all_zero_block_offset != 0)
+ data_offset += SPARSE_BLOCK_SIZE;
+ }
+ }
+
+ return true;
+}
+
static void
gcore_copy_callback (bfd *obfd, asection *osec)
{
@@ -599,8 +705,9 @@ gcore_copy_callback (bfd *obfd, asection *osec)
bfd_section_vma (osec)));
break;
}
- if (!bfd_set_section_contents (obfd, osec, memhunk.data (),
- offset, size))
+
+ if (!sparse_bfd_set_section_contents (obfd, osec, memhunk.data (),
+ offset, size))
{
warning (_("Failed to write corefile contents (%s)."),
bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
index 3f9ae48abf2..6c64d402502 100644
--- a/gdb/testsuite/gdb.base/bigcore.exp
+++ b/gdb/testsuite/gdb.base/bigcore.exp
@@ -43,23 +43,6 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
return -1
}
-# Run GDB on the bigcore program up-to where it will dump core.
-
-clean_restart ${binfile}
-gdb_test_no_output "set print sevenbit-strings"
-gdb_test_no_output "set width 0"
-
-# Get the core into the output directory.
-set_inferior_cwd_to_output_dir
-
-if {![runto_main]} {
- return 0
-}
-set print_core_line [gdb_get_line_number "Dump core"]
-gdb_test "tbreak $print_core_line"
-gdb_test continue ".*print_string.*"
-gdb_test next ".*0 = 0.*"
-
# Traverse part of bigcore's linked list of memory chunks (forward or
# backward), saving each chunk's address.
@@ -92,92 +75,11 @@ proc extract_heap { dir } {
}
return $heap
}
-set next_heap [extract_heap next]
-set prev_heap [extract_heap prev]
-
-# Save the total allocated size within GDB so that we can check
-# the core size later.
-gdb_test_no_output "set \$bytes_allocated = bytes_allocated" "save heap size"
-
-# Now create a core dump
-
-# Rename the core file to "TESTFILE.corefile" rather than just "core",
-# to avoid problems with sys admin types that like to regularly prune
-# all files named "core" from the system.
-
-# Some systems append "core" to the name of the program; others append
-# the name of the program to "core"; still others (like Linux, as of
-# May 2003) create cores named "core.PID".
-
-# Save the process ID. Some systems dump the core into core.PID.
-set inferior_pid [get_inferior_pid]
-
-# Dump core using SIGABRT
-set oldtimeout $timeout
-set timeout 600
-gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
-set timeout $oldtimeout
-
-# Find the corefile.
-set file [find_core_file $inferior_pid]
-if { $file != "" } {
- remote_exec build "mv $file $corefile"
-} else {
- untested "can't generate a core file"
- return 0
-}
-# Check that the corefile is plausibly large enough. We're trying to
-# detect the case where the operating system has truncated the file
-# just before signed wraparound. TCL, unfortunately, has a similar
-# problem - so use catch. It can handle the "bad" size but not
-# necessarily the "good" one. And we must use GDB for the comparison,
-# similarly.
-
-if {[catch {file size $corefile} core_size] == 0} {
- set core_ok 0
- gdb_test_multiple "print \$bytes_allocated < $core_size" "check core size" {
- -re " = 1\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 1
- }
- -re " = 0\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 0
- }
- }
-} {
- # Probably failed due to the TCL build having problems with very
- # large values. Since GDB uses a 64-bit off_t (when possible) it
- # shouldn't have this problem. Assume that things are going to
- # work. Without this assumption the test is skiped on systems
- # (such as i386 GNU/Linux with patched kernel) which do pass.
- pass "check core size"
- set core_ok 1
-}
-if {! $core_ok} {
- untested "check core size (system does not support large corefiles)"
- return 0
-}
-
-# Now load up that core file
-
-set test "load corefile"
-gdb_test_multiple "core $corefile" "$test" {
- -re "A program is being debugged already. Kill it. .y or n. " {
- send_gdb "y\n"
- exp_continue
- }
- -re "Core was generated by.*$gdb_prompt $" {
- pass "$test"
- }
-}
-
-# Finally, re-traverse bigcore's linked list, checking each chunk's
-# address against the executable. Don't use gdb_test_multiple as want
-# only one pass/fail. Don't use exp_continue as the regular
-# expression involving $heap needs to be re-evaluated for each new
-# response.
+# Re-traverse bigcore's linked list, checking each chunk's address
+# against the executable. Don't use gdb_test_multiple as want only
+# one pass/fail. Don't use exp_continue as the regular expression
+# involving $heap needs to be re-evaluated for each new response.
proc check_heap { dir heap } {
global gdb_prompt
@@ -208,5 +110,133 @@ proc check_heap { dir heap } {
}
}
-check_heap next $next_heap
-check_heap prev $prev_heap
+# The bulk of the testcase. DUMPER indicates who is supposed to dump
+# the core. It can be either "kernel", or "gdb".
+proc test {dumper} {
+ global binfile timeout corefile gdb_prompt
+
+ # Run GDB on the bigcore program up-to where it will dump core.
+
+ clean_restart ${binfile}
+ gdb_test_no_output "set print sevenbit-strings"
+ gdb_test_no_output "set width 0"
+
+ # Get the core into the output directory.
+ set_inferior_cwd_to_output_dir
+
+ if {![runto_main]} {
+ return 0
+ }
+ set print_core_line [gdb_get_line_number "Dump core"]
+ gdb_test "tbreak $print_core_line"
+ gdb_test continue ".*print_string.*"
+ gdb_test next ".*0 = 0.*"
+
+ set next_heap [extract_heap next]
+ set prev_heap [extract_heap prev]
+
+ # Save the total allocated size within GDB so that we can check
+ # the core size later.
+ gdb_test_no_output "set \$bytes_allocated = bytes_allocated" \
+ "save heap size"
+
+ # Now create a core dump.
+
+ if {$dumper == "kernel"} {
+ # Rename the core file to "TESTFILE.corefile.$dumper" rather
+ # than just "core", to avoid problems with sys admin types
+ # that like to regularly prune all files named "core" from the
+ # system.
+
+ # Some systems append "core" to the name of the program;
+ # others append the name of the program to "core"; still
+ # others (like Linux, as of May 2003) create cores named
+ # "core.PID".
+
+ # Save the process ID. Some systems dump the core into
+ # core.PID.
+ set inferior_pid [get_inferior_pid]
+
+ # Dump core using SIGABRT.
+ set oldtimeout $timeout
+ set timeout 600
+ gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
+ set timeout $oldtimeout
+
+ # Find the corefile.
+ set file [find_core_file $inferior_pid]
+ if { $file != "" } {
+ remote_exec build "mv $file $corefile.$dumper"
+ } else {
+ untested "can't generate a core file"
+ return 0
+ }
+ } elseif {$dumper == "gdb"} {
+ gdb_gcore_cmd "$corefile.$dumper" "gcore corefile"
+ } else {
+ error "unhandled dumper: $dumper"
+ }
+
+ # Check that the corefile is plausibly large enough. We're trying
+ # to detect the case where the operating system has truncated the
+ # file just before signed wraparound. TCL, unfortunately, has a
+ # similar problem - so use catch. It can handle the "bad" size
+ # but not necessarily the "good" one. And we must use GDB for the
+ # comparison, similarly.
+
+ if {[catch {file size $corefile.$dumper} core_size] == 0} {
+ set core_ok 0
+ gdb_test_multiple "print \$bytes_allocated < $core_size" \
+ "check core size" {
+ -re " = 1\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 1
+ }
+ -re " = 0\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 0
+ }
+ }
+ } {
+ # Probably failed due to the TCL build having problems with
+ # very large values. Since GDB uses a 64-bit off_t (when
+ # possible) it shouldn't have this problem. Assume that
+ # things are going to work. Without this assumption the test
+ # is skiped on systems (such as i386 GNU/Linux with patched
+ # kernel) which do pass.
+ pass "check core size"
+ set core_ok 1
+ }
+ if {! $core_ok} {
+ untested "check core size (system does not support large corefiles)"
+ return 0
+ }
+
+ # Now load up that core file.
+
+ set test "load corefile"
+ gdb_test_multiple "core $corefile.$dumper" "$test" {
+ -re "A program is being debugged already. Kill it. .y or n. " {
+ send_gdb "y\n"
+ exp_continue
+ }
+ -re "Core was generated by.*$gdb_prompt $" {
+ pass "$test"
+ }
+ }
+
+ # Finally, re-traverse bigcore's linked list, checking each
+ # chunk's address against the executable.
+
+ check_heap next $next_heap
+ check_heap prev $prev_heap
+}
+
+foreach_with_prefix dumper {kernel gdb} {
+ # GDB's gcore is too slow when testing with the extended-gdbserver
+ # board, since it requires reading all the inferior memory.
+ if {$dumper == "gdb" && [target_info gdb_protocol] != ""} {
+ continue
+ }
+ test $dumper
+}
base-commit: d01264b71654dae83a254cf030a5cf73b66b7fbb
--
2.43.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-15 18:27 [PATCH] Teach GDB to generate sparse core files (PR corefiles/31494) Pedro Alves
@ 2024-03-15 18:40 ` Eli Zaretskii
2024-03-18 13:29 ` [PATCH v2] " Pedro Alves
1 sibling, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2024-03-15 18:40 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, lancelot.six
> From: Pedro Alves <pedro@palves.net>
> Cc: Lancelot Six <lancelot.six@amd.com>
> Date: Fri, 15 Mar 2024 18:27:05 +0000
>
> gdb/NEWS | 4 +
> gdb/doc/gdb.texinfo | 3 +
> gdb/gcore.c | 115 +++++++++++++-
> gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
> 4 files changed, 252 insertions(+), 108 deletions(-)
The documentation parts of this are approved, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-15 18:27 [PATCH] Teach GDB to generate sparse core files (PR corefiles/31494) Pedro Alves
2024-03-15 18:40 ` Eli Zaretskii
@ 2024-03-18 13:29 ` Pedro Alves
2024-03-18 17:43 ` [PATCH v3] " Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2024-03-18 13:29 UTC (permalink / raw)
To: gdb-patches; +Cc: Lancelot Six
On 2024-03-15 18:27, Pedro Alves wrote:
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>
Lancelot, I didn't know whether I should keep the tag of not, since I modified the
patch. I kept it.
> +/* To generate sparse cores, we look at the data to write in chunks of
> + this size when considering whether to skip the write. Only if we
> + have a full block of this size with all zeros do we skip writing
> + it. A simpler algorithm that would try to skip all zeros would
> + result in potentially many more write/lseek syscalls, as normal
> + data is typically sprinkled with many small holes of zeros. */
> +#define SPARSE_BLOCK_SIZE 0x1000
I tweaked this comment a little to mention the memcmp efficiency.
> +/* Return true if we have a block full of zeros at DATA within the
> + [DATA, DATA+SIZE) buffer, false otherwise. */
> +
> +static bool
> +is_all_zero_block (const gdb_byte *data, size_t size)
> +{
> + if (size < SPARSE_BLOCK_SIZE)
> + return false;
I was looking at this, and realized that there is really no reason to force a write
of zeros to the file if it happens that the section is smaller than SPARSE_BLOCK_SIZE.
So I tweaked the code to do the memcmp on a min of size and SPARSE_BLOCK_SIZE.
And if I do that, then I need to return the size of the all-zero block considered, so
the function's prototype & name changed.
> +
> + /* A memcmp of a whole block is much faster than a simple for loop.
> + This makes a big difference, as with a for loop, this code would
> + dominate the performance and result in doubling the time to
> + generate a core, at the time of writing. With an optimized
> + memcmp, this doesn't even show up in the perf trace. */
> + static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
> + return memcmp (data, all_zero_block, SPARSE_BLOCK_SIZE) == 0;
> +}
> +
> +/* Find the next all-zero block at DATA+OFFSET within the [DATA,
> + DATA+SIZE) buffer. Returns the offset to the all-zero block if
> + found, or zero if not found. */
> +
> +static size_t
> +next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
> +{
Same for this function, I needed to return the size of the all-zero block
found.
> +/* Wrapper around bfd_set_section_contents that avoids writing
> + all-zero blocks to disk, so we create a sparse core file. */
> +
> +static bool
> +sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
> + const gdb_byte *data,
> + size_t sec_offset,
> + size_t size)
> +{
> + /* Note, we don't have to have special handling for the case of the
> + last memory region ending with zeros, because our caller always
> + writes out the note section after the memory/load sections. If
> + it didn't, we'd have to seek+write the last byte to make the file
> + size correct. (Or add an ftruncate abstraction to bfd and call
> + that.)
> +
> + If the blocks we skip are not aligned with the filesystem blocks,
> + on filesystems with fixed blocked size, we may waste a tiny bit
> + of file size, as the blocks that are adjacent to all-zero blocks
> + will have to include an amount of zeros. However, in practice,
> + sections have some alignment, and so SEC_OFFSET will be aligned
> + too, as our caller reads-in memory in chunks of SPARSE_BLOCK_SIZE
> + multiples. It's just not worth the bother to worry about
> + alignment here. */
And while I was hacking on the patch again, I realized how this comment about sections
being usually aligned is kind of bogus. It doesn't matter that a section's VMA is aligned.
What matters is the section's file position on disk. I took a look, and indeed, _that_ is
usually not aligned. I added code to align the writes/skips now. I originally thought
it would too complicated, but I found out how to make bfd compute the section file
offsets before writing anything. Just write 0 bytes:
bool
_bfd_elf_set_section_contents (bfd *abfd,
sec_ptr section,
const void *location,
file_ptr offset,
bfd_size_type count)
{
Elf_Internal_Shdr *hdr;
if (! abfd->output_has_begun
&& ! _bfd_elf_compute_section_file_positions (abfd, NULL))
return false;
if (!count)
return true;
...
I checked a few other backends, like e.g., coff_set_section_contents and they all do the same, always
before the count==0 check. This makes sense, as this way, any valid bfd_set_section_contents call
marks that output has begun.
> +
> + /* If we already know we have an all-zero block at the next
> + offset, we can skip calling is_all_zero_block for it
> + again. */
> + if (next_all_zero_block_offset != 0)
> + data_offset += SPARSE_BLOCK_SIZE;
This += is why I needed to get the size of the all-zero block found, as it can be less than
SPARSE_BLOCK_SIZE after the other changes.
Here's a v2, with Eli's reviewed-by added. Documentation hasn't changed.
---- >8 ----
From 461d9425e4043c82877b8784a3b26c7a9c95add9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 18 Mar 2024 13:16:10 +0000
Subject: [PATCH v2] Teach GDB to generate sparse core files (PR corefiles/31494)
This commit teaches GDB's gcore command to generate sparse core files
(if supported by the filesystem).
To create a sparse file, all you have to do is skip writing zeros to
the file, instead lseek'ing-ahead over them.
The sparse logic is applied when writing the memory sections, as
that's where the bulk of the data and the zeros are.
The commit also tweaks gdb.base/bigcore.exp to make it exercise
gdb-generated cores in addition to kernel-generated cores. We
couldn't do that before, because GDB's gcore on that test's program
would generate a multi-GB non-sparse core (16GB on my system).
After this commit, gdb.base/bigcore.exp generates, when testing with
GDB's gcore, a much smaller core file, roughly in line with what the
kernel produces:
real sizes:
$ du --hu testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
2.2M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
2.0M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
apparent sizes:
$ du --hu --apparent-size testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
Time to generate the core also goes down significantly. On my machine, I get:
when writing to an SSD, from 21.0s, down to 8.0s
when writing to an HDD, from 31.0s, down to 8.5s
The changes to gdb.base/bigcore.exp are smaller than they look at
first sight. It's basically mostly refactoring -- moving most of the
code to a new procedure which takes as argument who should dump the
core, and then calling the procedure twice. I purposedly did not
modernize any of the refactored code in this patch.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
Reviewed-By: Lancelot Six <lancelot.six@amd.com>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
---
gdb/NEWS | 4 +
gdb/doc/gdb.texinfo | 3 +
gdb/gcore.c | 177 ++++++++++++++++++++-
gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
4 files changed, 314 insertions(+), 108 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index d8ac0bb06a7..d1d25e4c24d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,10 @@ disassemble
command will now give an error. Previously the 'b' flag would
always override the 'r' flag.
+gcore
+generate-core-file
+ GDB now generates sparse core files, on systems that support it.
+
maintenance info line-table
Add an EPILOGUE-BEGIN column to the output of the command. It indicates
if the line is considered the start of the epilgoue, and thus a point at
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f093ee269e2..9224829bd93 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -13867,6 +13867,9 @@ Produce a core dump of the inferior process. The optional argument
specified, the file name defaults to @file{core.@var{pid}}, where
@var{pid} is the inferior process ID.
+If supported by the filesystem where the core is written to,
+@value{GDBN} generates a sparse core dump file.
+
Note that this command is implemented only for some systems (as of
this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 7c12aa3a777..a9303a99ab1 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -39,10 +39,21 @@
#include "gdbsupport/byte-vector.h"
#include "gdbsupport/scope-exit.h"
+/* To generate sparse cores, we look at the data to write in chunks of
+ this size when considering whether to skip the write. Only if we
+ have a full block of this size with all zeros do we skip writing
+ it. A simpler algorithm that would try to skip all zeros would
+ result in potentially many more write/lseek syscalls, as normal
+ data is typically sprinkled with many small holes of zeros. Also,
+ it's much more efficient to memcmp a block of data against an
+ all-zero buffer than to check each and every data byte against zero
+ one by one. */
+#define SPARSE_BLOCK_SIZE 0x1000
+
/* The largest amount of memory to read from the target at once. We
must throttle it to limit the amount of memory used by GDB during
generate-core-file for programs with large resident data. */
-#define MAX_COPY_BYTES (1024 * 1024)
+#define MAX_COPY_BYTES (256 * SPARSE_BLOCK_SIZE)
static const char *default_gcore_target (void);
static enum bfd_architecture default_gcore_arch (void);
@@ -98,7 +109,12 @@ write_gcore_file_1 (bfd *obfd)
bfd_set_section_alignment (note_sec, 0);
bfd_set_section_size (note_sec, note_size);
- /* Now create the memory/load sections. */
+ /* Now create the memory/load sections. Note
+ gcore_memory_sections's sparse logic is assuming that we'll
+ always write something afterwards, which we do: just below, we
+ write the note section. So there's no need for an ftruncate-like
+ call to grow the file to the right size if the last memory
+ sections were zeros and we skipped writing them. */
if (gcore_memory_sections (obfd) == 0)
error (_("gcore: failed to get corefile memory sections from target."));
@@ -567,6 +583,158 @@ objfile_find_memory_regions (struct target_ops *self,
return 0;
}
+/* Check if we have a block full of zeros at DATA within the [DATA,
+ DATA+SIZE) buffer. Returns the size of the all-zero block found.
+ Returns at most the minimum between SIZE and SPARSE_BLOCK_SIZE. */
+
+static size_t
+get_all_zero_block_size (const gdb_byte *data, size_t size)
+{
+ size = std::min (size, (size_t) SPARSE_BLOCK_SIZE);
+
+ /* A memcmp of a whole block is much faster than a simple for loop.
+ This makes a big difference, as with a for loop, this code would
+ dominate the performance and result in doubling the time to
+ generate a core, at the time of writing. With an optimized
+ memcmp, this doesn't even show up in the perf trace. */
+ static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
+ if (memcmp (data, all_zero_block, size) == 0)
+ return size;
+ return 0;
+}
+
+/* Basically a named-elements pair, used as return type of
+ find_next_all_zero_block. */
+
+struct offset_and_size
+{
+ size_t offset;
+ size_t size;
+};
+
+/* Find the next all-zero block at DATA+OFFSET within the [DATA,
+ DATA+SIZE) buffer. Returns the offset and the size of the all-zero
+ block if found, or zero if not found. */
+
+static offset_and_size
+find_next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
+{
+ for (; offset < size; offset += SPARSE_BLOCK_SIZE)
+ {
+ size_t zero_block_size
+ = get_all_zero_block_size (data + offset, size - offset);
+ if (zero_block_size != 0)
+ return {offset, zero_block_size};
+ }
+ return {0, 0};
+}
+
+/* Wrapper around bfd_set_section_contents that avoids writing
+ all-zero blocks to disk, so we create a sparse core file.
+ SKIP_ALIGN is a recursion helper -- if true, we'll skip aligning
+ the file position to SPARSE_BLOCK_SIZE. */
+
+static bool
+sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
+ const gdb_byte *data,
+ size_t sec_offset,
+ size_t size,
+ bool skip_align = false)
+{
+ /* Note, we don't have to have special handling for the case of the
+ last memory region ending with zeros, because our caller always
+ writes out the note section after the memory/load sections. If
+ it didn't, we'd have to seek+write the last byte to make the file
+ size correct. (Or add an ftruncate abstraction to bfd and call
+ that.) */
+
+ if (!skip_align)
+ {
+ /* Align the all-zero block search with SPARSE_BLOCK_SIZE, to
+ better align with filesystem blocks. If we find we're
+ misaligned, then write/skip the bytes needed to make us
+ aligned. We do that with (one level) recursion. */
+
+ /* We need to know the section's file offset on disk. We can
+ only look at it after the bfd's 'output_has_begun' flag has
+ been set, as bfd hasn't computed the file offsets
+ otherwise. */
+ if (!obfd->output_has_begun)
+ {
+ gdb_byte dummy = 0;
+
+ /* A write forces BFD to compute the bfd's section file
+ positions. Zero size works for that too. */
+ if (!bfd_set_section_contents (obfd, osec, &dummy, 0, 0))
+ return false;
+
+ gdb_assert (obfd->output_has_begun);
+ }
+
+ /* How much we need to write/skip in order to find the next
+ SPARSE_BLOCK_SIZE filepos-aligned block. */
+ size_t align_remainder
+ = (SPARSE_BLOCK_SIZE
+ - (osec->filepos + sec_offset) % SPARSE_BLOCK_SIZE);
+
+ /* How much we'll actually write in the recursion call. */
+ size_t align_write_size = std::min (size, align_remainder);
+
+ if (align_write_size != 0)
+ {
+ /* Recurse, skipping the alignment code. */
+ if (!sparse_bfd_set_section_contents (obfd, osec, data,
+ sec_offset,
+ align_write_size, true))
+ return false;
+
+ /* Skip over what we've written, and proceed with
+ assumes-aligned logic. */
+ data += align_write_size;
+ sec_offset += align_write_size;
+ size -= align_write_size;
+ }
+ }
+
+ size_t data_offset = 0;
+ while (data_offset < size)
+ {
+ size_t all_zero_block_size
+ = get_all_zero_block_size (data + data_offset, size - data_offset);
+ if (all_zero_block_size != 0)
+ data_offset += all_zero_block_size;
+ else
+ {
+ /* We have some non-zero data to write to file. Find the
+ next all-zero block within the data, and only write up to
+ it. */
+
+ offset_and_size next_all_zero_block
+ = find_next_all_zero_block (data,
+ data_offset + SPARSE_BLOCK_SIZE,
+ size);
+ size_t next_data_offset = (next_all_zero_block.offset == 0
+ ? size
+ : next_all_zero_block.offset);
+
+ if (!bfd_set_section_contents (obfd, osec, data + data_offset,
+ sec_offset + data_offset,
+ next_data_offset - data_offset))
+ return false;
+
+ data_offset = next_data_offset;
+
+ /* If we already know we have an all-zero block at the next
+ offset, we can skip calling get_all_zero_block_size for
+ it again. */
+ if (next_all_zero_block.offset != 0)
+ data_offset += next_all_zero_block.offset;
+ }
+ }
+
+ return true;
+}
+
static void
gcore_copy_callback (bfd *obfd, asection *osec)
{
@@ -599,8 +767,9 @@ gcore_copy_callback (bfd *obfd, asection *osec)
bfd_section_vma (osec)));
break;
}
- if (!bfd_set_section_contents (obfd, osec, memhunk.data (),
- offset, size))
+
+ if (!sparse_bfd_set_section_contents (obfd, osec, memhunk.data (),
+ offset, size))
{
warning (_("Failed to write corefile contents (%s)."),
bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
index 3f9ae48abf2..6c64d402502 100644
--- a/gdb/testsuite/gdb.base/bigcore.exp
+++ b/gdb/testsuite/gdb.base/bigcore.exp
@@ -43,23 +43,6 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
return -1
}
-# Run GDB on the bigcore program up-to where it will dump core.
-
-clean_restart ${binfile}
-gdb_test_no_output "set print sevenbit-strings"
-gdb_test_no_output "set width 0"
-
-# Get the core into the output directory.
-set_inferior_cwd_to_output_dir
-
-if {![runto_main]} {
- return 0
-}
-set print_core_line [gdb_get_line_number "Dump core"]
-gdb_test "tbreak $print_core_line"
-gdb_test continue ".*print_string.*"
-gdb_test next ".*0 = 0.*"
-
# Traverse part of bigcore's linked list of memory chunks (forward or
# backward), saving each chunk's address.
@@ -92,92 +75,11 @@ proc extract_heap { dir } {
}
return $heap
}
-set next_heap [extract_heap next]
-set prev_heap [extract_heap prev]
-
-# Save the total allocated size within GDB so that we can check
-# the core size later.
-gdb_test_no_output "set \$bytes_allocated = bytes_allocated" "save heap size"
-
-# Now create a core dump
-
-# Rename the core file to "TESTFILE.corefile" rather than just "core",
-# to avoid problems with sys admin types that like to regularly prune
-# all files named "core" from the system.
-
-# Some systems append "core" to the name of the program; others append
-# the name of the program to "core"; still others (like Linux, as of
-# May 2003) create cores named "core.PID".
-
-# Save the process ID. Some systems dump the core into core.PID.
-set inferior_pid [get_inferior_pid]
-
-# Dump core using SIGABRT
-set oldtimeout $timeout
-set timeout 600
-gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
-set timeout $oldtimeout
-
-# Find the corefile.
-set file [find_core_file $inferior_pid]
-if { $file != "" } {
- remote_exec build "mv $file $corefile"
-} else {
- untested "can't generate a core file"
- return 0
-}
-# Check that the corefile is plausibly large enough. We're trying to
-# detect the case where the operating system has truncated the file
-# just before signed wraparound. TCL, unfortunately, has a similar
-# problem - so use catch. It can handle the "bad" size but not
-# necessarily the "good" one. And we must use GDB for the comparison,
-# similarly.
-
-if {[catch {file size $corefile} core_size] == 0} {
- set core_ok 0
- gdb_test_multiple "print \$bytes_allocated < $core_size" "check core size" {
- -re " = 1\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 1
- }
- -re " = 0\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 0
- }
- }
-} {
- # Probably failed due to the TCL build having problems with very
- # large values. Since GDB uses a 64-bit off_t (when possible) it
- # shouldn't have this problem. Assume that things are going to
- # work. Without this assumption the test is skiped on systems
- # (such as i386 GNU/Linux with patched kernel) which do pass.
- pass "check core size"
- set core_ok 1
-}
-if {! $core_ok} {
- untested "check core size (system does not support large corefiles)"
- return 0
-}
-
-# Now load up that core file
-
-set test "load corefile"
-gdb_test_multiple "core $corefile" "$test" {
- -re "A program is being debugged already. Kill it. .y or n. " {
- send_gdb "y\n"
- exp_continue
- }
- -re "Core was generated by.*$gdb_prompt $" {
- pass "$test"
- }
-}
-
-# Finally, re-traverse bigcore's linked list, checking each chunk's
-# address against the executable. Don't use gdb_test_multiple as want
-# only one pass/fail. Don't use exp_continue as the regular
-# expression involving $heap needs to be re-evaluated for each new
-# response.
+# Re-traverse bigcore's linked list, checking each chunk's address
+# against the executable. Don't use gdb_test_multiple as want only
+# one pass/fail. Don't use exp_continue as the regular expression
+# involving $heap needs to be re-evaluated for each new response.
proc check_heap { dir heap } {
global gdb_prompt
@@ -208,5 +110,133 @@ proc check_heap { dir heap } {
}
}
-check_heap next $next_heap
-check_heap prev $prev_heap
+# The bulk of the testcase. DUMPER indicates who is supposed to dump
+# the core. It can be either "kernel", or "gdb".
+proc test {dumper} {
+ global binfile timeout corefile gdb_prompt
+
+ # Run GDB on the bigcore program up-to where it will dump core.
+
+ clean_restart ${binfile}
+ gdb_test_no_output "set print sevenbit-strings"
+ gdb_test_no_output "set width 0"
+
+ # Get the core into the output directory.
+ set_inferior_cwd_to_output_dir
+
+ if {![runto_main]} {
+ return 0
+ }
+ set print_core_line [gdb_get_line_number "Dump core"]
+ gdb_test "tbreak $print_core_line"
+ gdb_test continue ".*print_string.*"
+ gdb_test next ".*0 = 0.*"
+
+ set next_heap [extract_heap next]
+ set prev_heap [extract_heap prev]
+
+ # Save the total allocated size within GDB so that we can check
+ # the core size later.
+ gdb_test_no_output "set \$bytes_allocated = bytes_allocated" \
+ "save heap size"
+
+ # Now create a core dump.
+
+ if {$dumper == "kernel"} {
+ # Rename the core file to "TESTFILE.corefile.$dumper" rather
+ # than just "core", to avoid problems with sys admin types
+ # that like to regularly prune all files named "core" from the
+ # system.
+
+ # Some systems append "core" to the name of the program;
+ # others append the name of the program to "core"; still
+ # others (like Linux, as of May 2003) create cores named
+ # "core.PID".
+
+ # Save the process ID. Some systems dump the core into
+ # core.PID.
+ set inferior_pid [get_inferior_pid]
+
+ # Dump core using SIGABRT.
+ set oldtimeout $timeout
+ set timeout 600
+ gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
+ set timeout $oldtimeout
+
+ # Find the corefile.
+ set file [find_core_file $inferior_pid]
+ if { $file != "" } {
+ remote_exec build "mv $file $corefile.$dumper"
+ } else {
+ untested "can't generate a core file"
+ return 0
+ }
+ } elseif {$dumper == "gdb"} {
+ gdb_gcore_cmd "$corefile.$dumper" "gcore corefile"
+ } else {
+ error "unhandled dumper: $dumper"
+ }
+
+ # Check that the corefile is plausibly large enough. We're trying
+ # to detect the case where the operating system has truncated the
+ # file just before signed wraparound. TCL, unfortunately, has a
+ # similar problem - so use catch. It can handle the "bad" size
+ # but not necessarily the "good" one. And we must use GDB for the
+ # comparison, similarly.
+
+ if {[catch {file size $corefile.$dumper} core_size] == 0} {
+ set core_ok 0
+ gdb_test_multiple "print \$bytes_allocated < $core_size" \
+ "check core size" {
+ -re " = 1\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 1
+ }
+ -re " = 0\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 0
+ }
+ }
+ } {
+ # Probably failed due to the TCL build having problems with
+ # very large values. Since GDB uses a 64-bit off_t (when
+ # possible) it shouldn't have this problem. Assume that
+ # things are going to work. Without this assumption the test
+ # is skiped on systems (such as i386 GNU/Linux with patched
+ # kernel) which do pass.
+ pass "check core size"
+ set core_ok 1
+ }
+ if {! $core_ok} {
+ untested "check core size (system does not support large corefiles)"
+ return 0
+ }
+
+ # Now load up that core file.
+
+ set test "load corefile"
+ gdb_test_multiple "core $corefile.$dumper" "$test" {
+ -re "A program is being debugged already. Kill it. .y or n. " {
+ send_gdb "y\n"
+ exp_continue
+ }
+ -re "Core was generated by.*$gdb_prompt $" {
+ pass "$test"
+ }
+ }
+
+ # Finally, re-traverse bigcore's linked list, checking each
+ # chunk's address against the executable.
+
+ check_heap next $next_heap
+ check_heap prev $prev_heap
+}
+
+foreach_with_prefix dumper {kernel gdb} {
+ # GDB's gcore is too slow when testing with the extended-gdbserver
+ # board, since it requires reading all the inferior memory.
+ if {$dumper == "gdb" && [target_info gdb_protocol] != ""} {
+ continue
+ }
+ test $dumper
+}
base-commit: d0eb2625bff1387744304bdc70ec0a85a20b8a3f
--
2.43.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-18 13:29 ` [PATCH v2] " Pedro Alves
@ 2024-03-18 17:43 ` Pedro Alves
2024-03-19 16:41 ` John Baldwin
2024-03-21 21:27 ` Lancelot SIX
0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2024-03-18 17:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Lancelot Six
On 2024-03-18 13:29, Pedro Alves wrote:
> On 2024-03-15 18:27, Pedro Alves wrote:
> + /* If we already know we have an all-zero block at the next
> + offset, we can skip calling get_all_zero_block_size for
> + it again. */
> + if (next_all_zero_block.offset != 0)
> + data_offset += next_all_zero_block.offset;
Err, all the effort to pass down the size, only to typo and not use it... Sigh.
That last line should be:
data_offset += next_all_zero_block.size;
Here's the corrected patch...
From adb681ce583fa640c4fb6883a827f3ab6b28b1c0 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 18 Mar 2024 13:16:10 +0000
Subject: [PATCH v3] Teach GDB to generate sparse core files (PR
corefiles/31494)
This commit teaches GDB's gcore command to generate sparse core files
(if supported by the filesystem).
To create a sparse file, all you have to do is skip writing zeros to
the file, instead lseek'ing-ahead over them.
The sparse logic is applied when writing the memory sections, as
that's where the bulk of the data and the zeros are.
The commit also tweaks gdb.base/bigcore.exp to make it exercise
gdb-generated cores in addition to kernel-generated cores. We
couldn't do that before, because GDB's gcore on that test's program
would generate a multi-GB non-sparse core (16GB on my system).
After this commit, gdb.base/bigcore.exp generates, when testing with
GDB's gcore, a much smaller core file, roughly in line with what the
kernel produces:
real sizes:
$ du --hu testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
2.2M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
2.0M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
apparent sizes:
$ du --hu --apparent-size testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
Time to generate the core also goes down significantly. On my machine, I get:
when writing to an SSD, from 21.0s, down to 8.0s
when writing to an HDD, from 31.0s, down to 8.5s
The changes to gdb.base/bigcore.exp are smaller than they look at
first sight. It's basically mostly refactoring -- moving most of the
code to a new procedure which takes as argument who should dump the
core, and then calling the procedure twice. I purposedly did not
modernize any of the refactored code in this patch.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
Reviewed-By: Lancelot Six <lancelot.six@amd.com>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
---
gdb/NEWS | 4 +
gdb/doc/gdb.texinfo | 3 +
gdb/gcore.c | 177 ++++++++++++++++++++-
gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
4 files changed, 314 insertions(+), 108 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index d8ac0bb06a7..d1d25e4c24d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,10 @@ disassemble
command will now give an error. Previously the 'b' flag would
always override the 'r' flag.
+gcore
+generate-core-file
+ GDB now generates sparse core files, on systems that support it.
+
maintenance info line-table
Add an EPILOGUE-BEGIN column to the output of the command. It indicates
if the line is considered the start of the epilgoue, and thus a point at
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f093ee269e2..9224829bd93 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -13867,6 +13867,9 @@ Produce a core dump of the inferior process. The optional argument
specified, the file name defaults to @file{core.@var{pid}}, where
@var{pid} is the inferior process ID.
+If supported by the filesystem where the core is written to,
+@value{GDBN} generates a sparse core dump file.
+
Note that this command is implemented only for some systems (as of
this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 7c12aa3a777..23e8066745a 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -39,10 +39,21 @@
#include "gdbsupport/byte-vector.h"
#include "gdbsupport/scope-exit.h"
+/* To generate sparse cores, we look at the data to write in chunks of
+ this size when considering whether to skip the write. Only if we
+ have a full block of this size with all zeros do we skip writing
+ it. A simpler algorithm that would try to skip all zeros would
+ result in potentially many more write/lseek syscalls, as normal
+ data is typically sprinkled with many small holes of zeros. Also,
+ it's much more efficient to memcmp a block of data against an
+ all-zero buffer than to check each and every data byte against zero
+ one by one. */
+#define SPARSE_BLOCK_SIZE 0x1000
+
/* The largest amount of memory to read from the target at once. We
must throttle it to limit the amount of memory used by GDB during
generate-core-file for programs with large resident data. */
-#define MAX_COPY_BYTES (1024 * 1024)
+#define MAX_COPY_BYTES (256 * SPARSE_BLOCK_SIZE)
static const char *default_gcore_target (void);
static enum bfd_architecture default_gcore_arch (void);
@@ -98,7 +109,12 @@ write_gcore_file_1 (bfd *obfd)
bfd_set_section_alignment (note_sec, 0);
bfd_set_section_size (note_sec, note_size);
- /* Now create the memory/load sections. */
+ /* Now create the memory/load sections. Note
+ gcore_memory_sections's sparse logic is assuming that we'll
+ always write something afterwards, which we do: just below, we
+ write the note section. So there's no need for an ftruncate-like
+ call to grow the file to the right size if the last memory
+ sections were zeros and we skipped writing them. */
if (gcore_memory_sections (obfd) == 0)
error (_("gcore: failed to get corefile memory sections from target."));
@@ -567,6 +583,158 @@ objfile_find_memory_regions (struct target_ops *self,
return 0;
}
+/* Check if we have a block full of zeros at DATA within the [DATA,
+ DATA+SIZE) buffer. Returns the size of the all-zero block found.
+ Returns at most the minimum between SIZE and SPARSE_BLOCK_SIZE. */
+
+static size_t
+get_all_zero_block_size (const gdb_byte *data, size_t size)
+{
+ size = std::min (size, (size_t) SPARSE_BLOCK_SIZE);
+
+ /* A memcmp of a whole block is much faster than a simple for loop.
+ This makes a big difference, as with a for loop, this code would
+ dominate the performance and result in doubling the time to
+ generate a core, at the time of writing. With an optimized
+ memcmp, this doesn't even show up in the perf trace. */
+ static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
+ if (memcmp (data, all_zero_block, size) == 0)
+ return size;
+ return 0;
+}
+
+/* Basically a named-elements pair, used as return type of
+ find_next_all_zero_block. */
+
+struct offset_and_size
+{
+ size_t offset;
+ size_t size;
+};
+
+/* Find the next all-zero block at DATA+OFFSET within the [DATA,
+ DATA+SIZE) buffer. Returns the offset and the size of the all-zero
+ block if found, or zero if not found. */
+
+static offset_and_size
+find_next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
+{
+ for (; offset < size; offset += SPARSE_BLOCK_SIZE)
+ {
+ size_t zero_block_size
+ = get_all_zero_block_size (data + offset, size - offset);
+ if (zero_block_size != 0)
+ return {offset, zero_block_size};
+ }
+ return {0, 0};
+}
+
+/* Wrapper around bfd_set_section_contents that avoids writing
+ all-zero blocks to disk, so we create a sparse core file.
+ SKIP_ALIGN is a recursion helper -- if true, we'll skip aligning
+ the file position to SPARSE_BLOCK_SIZE. */
+
+static bool
+sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
+ const gdb_byte *data,
+ size_t sec_offset,
+ size_t size,
+ bool skip_align = false)
+{
+ /* Note, we don't have to have special handling for the case of the
+ last memory region ending with zeros, because our caller always
+ writes out the note section after the memory/load sections. If
+ it didn't, we'd have to seek+write the last byte to make the file
+ size correct. (Or add an ftruncate abstraction to bfd and call
+ that.) */
+
+ if (!skip_align)
+ {
+ /* Align the all-zero block search with SPARSE_BLOCK_SIZE, to
+ better align with filesystem blocks. If we find we're
+ misaligned, then write/skip the bytes needed to make us
+ aligned. We do that with (one level) recursion. */
+
+ /* We need to know the section's file offset on disk. We can
+ only look at it after the bfd's 'output_has_begun' flag has
+ been set, as bfd hasn't computed the file offsets
+ otherwise. */
+ if (!obfd->output_has_begun)
+ {
+ gdb_byte dummy = 0;
+
+ /* A write forces BFD to compute the bfd's section file
+ positions. Zero size works for that too. */
+ if (!bfd_set_section_contents (obfd, osec, &dummy, 0, 0))
+ return false;
+
+ gdb_assert (obfd->output_has_begun);
+ }
+
+ /* How much we need to write/skip in order to find the next
+ SPARSE_BLOCK_SIZE filepos-aligned block. */
+ size_t align_remainder
+ = (SPARSE_BLOCK_SIZE
+ - (osec->filepos + sec_offset) % SPARSE_BLOCK_SIZE);
+
+ /* How much we'll actually write in the recursion call. */
+ size_t align_write_size = std::min (size, align_remainder);
+
+ if (align_write_size != 0)
+ {
+ /* Recurse, skipping the alignment code. */
+ if (!sparse_bfd_set_section_contents (obfd, osec, data,
+ sec_offset,
+ align_write_size, true))
+ return false;
+
+ /* Skip over what we've written, and proceed with
+ assumes-aligned logic. */
+ data += align_write_size;
+ sec_offset += align_write_size;
+ size -= align_write_size;
+ }
+ }
+
+ size_t data_offset = 0;
+ while (data_offset < size)
+ {
+ size_t all_zero_block_size
+ = get_all_zero_block_size (data + data_offset, size - data_offset);
+ if (all_zero_block_size != 0)
+ data_offset += all_zero_block_size;
+ else
+ {
+ /* We have some non-zero data to write to file. Find the
+ next all-zero block within the data, and only write up to
+ it. */
+
+ offset_and_size next_all_zero_block
+ = find_next_all_zero_block (data,
+ data_offset + SPARSE_BLOCK_SIZE,
+ size);
+ size_t next_data_offset = (next_all_zero_block.offset == 0
+ ? size
+ : next_all_zero_block.offset);
+
+ if (!bfd_set_section_contents (obfd, osec, data + data_offset,
+ sec_offset + data_offset,
+ next_data_offset - data_offset))
+ return false;
+
+ data_offset = next_data_offset;
+
+ /* If we already know we have an all-zero block at the next
+ offset, we can skip calling get_all_zero_block_size for
+ it again. */
+ if (next_all_zero_block.offset != 0)
+ data_offset += next_all_zero_block.size;
+ }
+ }
+
+ return true;
+}
+
static void
gcore_copy_callback (bfd *obfd, asection *osec)
{
@@ -599,8 +767,9 @@ gcore_copy_callback (bfd *obfd, asection *osec)
bfd_section_vma (osec)));
break;
}
- if (!bfd_set_section_contents (obfd, osec, memhunk.data (),
- offset, size))
+
+ if (!sparse_bfd_set_section_contents (obfd, osec, memhunk.data (),
+ offset, size))
{
warning (_("Failed to write corefile contents (%s)."),
bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
index 3f9ae48abf2..6c64d402502 100644
--- a/gdb/testsuite/gdb.base/bigcore.exp
+++ b/gdb/testsuite/gdb.base/bigcore.exp
@@ -43,23 +43,6 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
return -1
}
-# Run GDB on the bigcore program up-to where it will dump core.
-
-clean_restart ${binfile}
-gdb_test_no_output "set print sevenbit-strings"
-gdb_test_no_output "set width 0"
-
-# Get the core into the output directory.
-set_inferior_cwd_to_output_dir
-
-if {![runto_main]} {
- return 0
-}
-set print_core_line [gdb_get_line_number "Dump core"]
-gdb_test "tbreak $print_core_line"
-gdb_test continue ".*print_string.*"
-gdb_test next ".*0 = 0.*"
-
# Traverse part of bigcore's linked list of memory chunks (forward or
# backward), saving each chunk's address.
@@ -92,92 +75,11 @@ proc extract_heap { dir } {
}
return $heap
}
-set next_heap [extract_heap next]
-set prev_heap [extract_heap prev]
-
-# Save the total allocated size within GDB so that we can check
-# the core size later.
-gdb_test_no_output "set \$bytes_allocated = bytes_allocated" "save heap size"
-
-# Now create a core dump
-
-# Rename the core file to "TESTFILE.corefile" rather than just "core",
-# to avoid problems with sys admin types that like to regularly prune
-# all files named "core" from the system.
-
-# Some systems append "core" to the name of the program; others append
-# the name of the program to "core"; still others (like Linux, as of
-# May 2003) create cores named "core.PID".
-
-# Save the process ID. Some systems dump the core into core.PID.
-set inferior_pid [get_inferior_pid]
-
-# Dump core using SIGABRT
-set oldtimeout $timeout
-set timeout 600
-gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
-set timeout $oldtimeout
-
-# Find the corefile.
-set file [find_core_file $inferior_pid]
-if { $file != "" } {
- remote_exec build "mv $file $corefile"
-} else {
- untested "can't generate a core file"
- return 0
-}
-# Check that the corefile is plausibly large enough. We're trying to
-# detect the case where the operating system has truncated the file
-# just before signed wraparound. TCL, unfortunately, has a similar
-# problem - so use catch. It can handle the "bad" size but not
-# necessarily the "good" one. And we must use GDB for the comparison,
-# similarly.
-
-if {[catch {file size $corefile} core_size] == 0} {
- set core_ok 0
- gdb_test_multiple "print \$bytes_allocated < $core_size" "check core size" {
- -re " = 1\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 1
- }
- -re " = 0\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 0
- }
- }
-} {
- # Probably failed due to the TCL build having problems with very
- # large values. Since GDB uses a 64-bit off_t (when possible) it
- # shouldn't have this problem. Assume that things are going to
- # work. Without this assumption the test is skiped on systems
- # (such as i386 GNU/Linux with patched kernel) which do pass.
- pass "check core size"
- set core_ok 1
-}
-if {! $core_ok} {
- untested "check core size (system does not support large corefiles)"
- return 0
-}
-
-# Now load up that core file
-
-set test "load corefile"
-gdb_test_multiple "core $corefile" "$test" {
- -re "A program is being debugged already. Kill it. .y or n. " {
- send_gdb "y\n"
- exp_continue
- }
- -re "Core was generated by.*$gdb_prompt $" {
- pass "$test"
- }
-}
-
-# Finally, re-traverse bigcore's linked list, checking each chunk's
-# address against the executable. Don't use gdb_test_multiple as want
-# only one pass/fail. Don't use exp_continue as the regular
-# expression involving $heap needs to be re-evaluated for each new
-# response.
+# Re-traverse bigcore's linked list, checking each chunk's address
+# against the executable. Don't use gdb_test_multiple as want only
+# one pass/fail. Don't use exp_continue as the regular expression
+# involving $heap needs to be re-evaluated for each new response.
proc check_heap { dir heap } {
global gdb_prompt
@@ -208,5 +110,133 @@ proc check_heap { dir heap } {
}
}
-check_heap next $next_heap
-check_heap prev $prev_heap
+# The bulk of the testcase. DUMPER indicates who is supposed to dump
+# the core. It can be either "kernel", or "gdb".
+proc test {dumper} {
+ global binfile timeout corefile gdb_prompt
+
+ # Run GDB on the bigcore program up-to where it will dump core.
+
+ clean_restart ${binfile}
+ gdb_test_no_output "set print sevenbit-strings"
+ gdb_test_no_output "set width 0"
+
+ # Get the core into the output directory.
+ set_inferior_cwd_to_output_dir
+
+ if {![runto_main]} {
+ return 0
+ }
+ set print_core_line [gdb_get_line_number "Dump core"]
+ gdb_test "tbreak $print_core_line"
+ gdb_test continue ".*print_string.*"
+ gdb_test next ".*0 = 0.*"
+
+ set next_heap [extract_heap next]
+ set prev_heap [extract_heap prev]
+
+ # Save the total allocated size within GDB so that we can check
+ # the core size later.
+ gdb_test_no_output "set \$bytes_allocated = bytes_allocated" \
+ "save heap size"
+
+ # Now create a core dump.
+
+ if {$dumper == "kernel"} {
+ # Rename the core file to "TESTFILE.corefile.$dumper" rather
+ # than just "core", to avoid problems with sys admin types
+ # that like to regularly prune all files named "core" from the
+ # system.
+
+ # Some systems append "core" to the name of the program;
+ # others append the name of the program to "core"; still
+ # others (like Linux, as of May 2003) create cores named
+ # "core.PID".
+
+ # Save the process ID. Some systems dump the core into
+ # core.PID.
+ set inferior_pid [get_inferior_pid]
+
+ # Dump core using SIGABRT.
+ set oldtimeout $timeout
+ set timeout 600
+ gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
+ set timeout $oldtimeout
+
+ # Find the corefile.
+ set file [find_core_file $inferior_pid]
+ if { $file != "" } {
+ remote_exec build "mv $file $corefile.$dumper"
+ } else {
+ untested "can't generate a core file"
+ return 0
+ }
+ } elseif {$dumper == "gdb"} {
+ gdb_gcore_cmd "$corefile.$dumper" "gcore corefile"
+ } else {
+ error "unhandled dumper: $dumper"
+ }
+
+ # Check that the corefile is plausibly large enough. We're trying
+ # to detect the case where the operating system has truncated the
+ # file just before signed wraparound. TCL, unfortunately, has a
+ # similar problem - so use catch. It can handle the "bad" size
+ # but not necessarily the "good" one. And we must use GDB for the
+ # comparison, similarly.
+
+ if {[catch {file size $corefile.$dumper} core_size] == 0} {
+ set core_ok 0
+ gdb_test_multiple "print \$bytes_allocated < $core_size" \
+ "check core size" {
+ -re " = 1\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 1
+ }
+ -re " = 0\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 0
+ }
+ }
+ } {
+ # Probably failed due to the TCL build having problems with
+ # very large values. Since GDB uses a 64-bit off_t (when
+ # possible) it shouldn't have this problem. Assume that
+ # things are going to work. Without this assumption the test
+ # is skiped on systems (such as i386 GNU/Linux with patched
+ # kernel) which do pass.
+ pass "check core size"
+ set core_ok 1
+ }
+ if {! $core_ok} {
+ untested "check core size (system does not support large corefiles)"
+ return 0
+ }
+
+ # Now load up that core file.
+
+ set test "load corefile"
+ gdb_test_multiple "core $corefile.$dumper" "$test" {
+ -re "A program is being debugged already. Kill it. .y or n. " {
+ send_gdb "y\n"
+ exp_continue
+ }
+ -re "Core was generated by.*$gdb_prompt $" {
+ pass "$test"
+ }
+ }
+
+ # Finally, re-traverse bigcore's linked list, checking each
+ # chunk's address against the executable.
+
+ check_heap next $next_heap
+ check_heap prev $prev_heap
+}
+
+foreach_with_prefix dumper {kernel gdb} {
+ # GDB's gcore is too slow when testing with the extended-gdbserver
+ # board, since it requires reading all the inferior memory.
+ if {$dumper == "gdb" && [target_info gdb_protocol] != ""} {
+ continue
+ }
+ test $dumper
+}
base-commit: d0eb2625bff1387744304bdc70ec0a85a20b8a3f
--
2.43.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-18 17:43 ` [PATCH v3] " Pedro Alves
@ 2024-03-19 16:41 ` John Baldwin
2024-03-21 21:27 ` Lancelot SIX
1 sibling, 0 replies; 9+ messages in thread
From: John Baldwin @ 2024-03-19 16:41 UTC (permalink / raw)
To: Pedro Alves, gdb-patches; +Cc: Lancelot Six
On 3/18/24 10:43 AM, Pedro Alves wrote:
> On 2024-03-18 13:29, Pedro Alves wrote:
>> On 2024-03-15 18:27, Pedro Alves wrote:
>
>> + /* If we already know we have an all-zero block at the next
>> + offset, we can skip calling get_all_zero_block_size for
>> + it again. */
>> + if (next_all_zero_block.offset != 0)
>> + data_offset += next_all_zero_block.offset;
>
> Err, all the effort to pass down the size, only to typo and not use it... Sigh.
> That last line should be:
>
> data_offset += next_all_zero_block.size;
>
> Here's the corrected patch...
>
> From adb681ce583fa640c4fb6883a827f3ab6b28b1c0 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Mon, 18 Mar 2024 13:16:10 +0000
> Subject: [PATCH v3] Teach GDB to generate sparse core files (PR
> corefiles/31494)
>
> This commit teaches GDB's gcore command to generate sparse core files
> (if supported by the filesystem).
>
> To create a sparse file, all you have to do is skip writing zeros to
> the file, instead lseek'ing-ahead over them.
>
> The sparse logic is applied when writing the memory sections, as
> that's where the bulk of the data and the zeros are.
>
> The commit also tweaks gdb.base/bigcore.exp to make it exercise
> gdb-generated cores in addition to kernel-generated cores. We
> couldn't do that before, because GDB's gcore on that test's program
> would generate a multi-GB non-sparse core (16GB on my system).
>
> After this commit, gdb.base/bigcore.exp generates, when testing with
> GDB's gcore, a much smaller core file, roughly in line with what the
> kernel produces:
>
> real sizes:
>
> $ du --hu testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
> 2.2M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
> 2.0M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
>
> apparent sizes:
>
> $ du --hu --apparent-size testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
> 16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
> 16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
>
> Time to generate the core also goes down significantly. On my machine, I get:
>
> when writing to an SSD, from 21.0s, down to 8.0s
> when writing to an HDD, from 31.0s, down to 8.5s
>
> The changes to gdb.base/bigcore.exp are smaller than they look at
> first sight. It's basically mostly refactoring -- moving most of the
> code to a new procedure which takes as argument who should dump the
> core, and then calling the procedure twice. I purposedly did not
> modernize any of the refactored code in this patch.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
Looks ok to me in general.
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
> +
> + size_t data_offset = 0;
> + while (data_offset < size)
> + {
> + size_t all_zero_block_size
> + = get_all_zero_block_size (data + data_offset, size - data_offset);
> + if (all_zero_block_size != 0)
> + data_offset += all_zero_block_size;
> + else
I would maybe use a continue here to avoid having to indent the large else
block, but either way is fine.
> + {
> + /* We have some non-zero data to write to file. Find the
> + next all-zero block within the data, and only write up to
> + it. */
> +
> + offset_and_size next_all_zero_block
> + = find_next_all_zero_block (data,
> + data_offset + SPARSE_BLOCK_SIZE,
> + size);
> + size_t next_data_offset = (next_all_zero_block.offset == 0
> + ? size
> + : next_all_zero_block.offset);
> +
> + if (!bfd_set_section_contents (obfd, osec, data + data_offset,
> + sec_offset + data_offset,
> + next_data_offset - data_offset))
> + return false;
> +
> + data_offset = next_data_offset;
> +
> + /* If we already know we have an all-zero block at the next
> + offset, we can skip calling get_all_zero_block_size for
> + it again. */
> + if (next_all_zero_block.offset != 0)
> + data_offset += next_all_zero_block.size;
> + }
> + }
> +
> + return true;
> +}
> +
--
John Baldwin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-18 17:43 ` [PATCH v3] " Pedro Alves
2024-03-19 16:41 ` John Baldwin
@ 2024-03-21 21:27 ` Lancelot SIX
2024-03-21 23:14 ` [PATCH v4] " Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: Lancelot SIX @ 2024-03-21 21:27 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Hi,
I have a couple of small comments below.
On 18/03/2024 17:43, Pedro Alves wrote:
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> @@ -567,6 +583,158 @@ objfile_find_memory_regions (struct target_ops *self,
> return 0;
> }
>
> +/* Check if we have a block full of zeros at DATA within the [DATA,
> + DATA+SIZE) buffer. Returns the size of the all-zero block found.
> + Returns at most the minimum between SIZE and SPARSE_BLOCK_SIZE. */
> +
> +static size_t
> +get_all_zero_block_size (const gdb_byte *data, size_t size)
> +{
> + size = std::min (size, (size_t) SPARSE_BLOCK_SIZE);
> +
> + /* A memcmp of a whole block is much faster than a simple for loop.
> + This makes a big difference, as with a for loop, this code would
> + dominate the performance and result in doubling the time to
> + generate a core, at the time of writing. With an optimized
> + memcmp, this doesn't even show up in the perf trace. */
> + static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
> + if (memcmp (data, all_zero_block, size) == 0)
> + return size;
> + return 0;
> +}
> +
> +/* Basically a named-elements pair, used as return type of
> + find_next_all_zero_block. */
> +
> +struct offset_and_size
> +{
> + size_t offset;
> + size_t size;
> +};
> +
> +/* Find the next all-zero block at DATA+OFFSET within the [DATA,
> + DATA+SIZE) buffer. Returns the offset and the size of the all-zero
> + block if found, or zero if not found. */
> +
> +static offset_and_size
> +find_next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
> +{
> + for (; offset < size; offset += SPARSE_BLOCK_SIZE)
> + {
> + size_t zero_block_size
> + = get_all_zero_block_size (data + offset, size - offset);
> + if (zero_block_size != 0)
> + return {offset, zero_block_size};
> + }
> + return {0, 0};
> +}
> +
> +/* Wrapper around bfd_set_section_contents that avoids writing
> + all-zero blocks to disk, so we create a sparse core file.
> + SKIP_ALIGN is a recursion helper -- if true, we'll skip aligning
> + the file position to SPARSE_BLOCK_SIZE. */
> +
> +static bool
> +sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
> + const gdb_byte *data,
> + size_t sec_offset,
> + size_t size,
> + bool skip_align = false)
> +{
> + /* Note, we don't have to have special handling for the case of the
> + last memory region ending with zeros, because our caller always
> + writes out the note section after the memory/load sections. If
> + it didn't, we'd have to seek+write the last byte to make the file
> + size correct. (Or add an ftruncate abstraction to bfd and call
> + that.) */
> +
> + if (!skip_align)
> + {
> + /* Align the all-zero block search with SPARSE_BLOCK_SIZE, to
> + better align with filesystem blocks. If we find we're
> + misaligned, then write/skip the bytes needed to make us
> + aligned. We do that with (one level) recursion. */
> +
> + /* We need to know the section's file offset on disk. We can
> + only look at it after the bfd's 'output_has_begun' flag has
> + been set, as bfd hasn't computed the file offsets
> + otherwise. */
> + if (!obfd->output_has_begun)
> + {
> + gdb_byte dummy = 0;
> +
> + /* A write forces BFD to compute the bfd's section file
> + positions. Zero size works for that too. */
> + if (!bfd_set_section_contents (obfd, osec, &dummy, 0, 0))
> + return false;
> +
> + gdb_assert (obfd->output_has_begun);
> + }
> +
> + /* How much we need to write/skip in order to find the next
> + SPARSE_BLOCK_SIZE filepos-aligned block. */
> + size_t align_remainder
> + = (SPARSE_BLOCK_SIZE
> + - (osec->filepos + sec_offset) % SPARSE_BLOCK_SIZE);
> +
> + /* How much we'll actually write in the recursion call. */
> + size_t align_write_size = std::min (size, align_remainder);
> +
> + if (align_write_size != 0)
I think at this point align_write_size can be SPARSE_BLOCK_SIZE (i.e.
sec_offset lands at a SPARSE_BLOCK_SIZE boundary in the underlying
filesystem). If that's the case, and data+sec_offset starts with block
of 0s, you'll write it to disk needlessly. Not a big deal, but I'd go for:
if (align_write_size % SPARSE_BLOCK_SIZE != 0)
> + {
> + /* Recurse, skipping the alignment code. */
> + if (!sparse_bfd_set_section_contents (obfd, osec, data,
> + sec_offset,
> + align_write_size, true))
> + return false;
> +
> + /* Skip over what we've written, and proceed with
> + assumes-aligned logic. */
> + data += align_write_size;
> + sec_offset += align_write_size;
> + size -= align_write_size;
> + }
> + }
> +
> + size_t data_offset = 0;
Just because that got me to think while reading, having the first part
of the procedure update data/sec_offset/size and the second part of the
procedure update data_offset seems a bit inconsistent.
I would probably move the declaration of data_offset at the very
begining of the procedure update it consistently:
size_t data_offset = 0;
if (!skip_align)
{
[…]
if (align_write_size % SPARSE_BLOCK_SIZE != 0)
{
[…]
data_offset += align_write_size;
}
}
while (data_offset < size)
[…]
> + while (data_offset < size)
> + {
> + size_t all_zero_block_size
> + = get_all_zero_block_size (data + data_offset, size - data_offset);
> + if (all_zero_block_size != 0)
> + data_offset += all_zero_block_size;
> + else
> + {
> + /* We have some non-zero data to write to file. Find the
> + next all-zero block within the data, and only write up to
> + it. */
> +
> + offset_and_size next_all_zero_block
> + = find_next_all_zero_block (data,
> + data_offset + SPARSE_BLOCK_SIZE,
> + size);
> + size_t next_data_offset = (next_all_zero_block.offset == 0
> + ? size
> + : next_all_zero_block.offset);
> +
> + if (!bfd_set_section_contents (obfd, osec, data + data_offset,
> + sec_offset + data_offset,
> + next_data_offset - data_offset))
> + return false;
> +
> + data_offset = next_data_offset;
> +
> + /* If we already know we have an all-zero block at the next
> + offset, we can skip calling get_all_zero_block_size for
> + it again. */
> + if (next_all_zero_block.offset != 0)
> + data_offset += next_all_zero_block.size;
> + }
> + }
> +
> + return true;
> +}
> +
> static void
> gcore_copy_callback (bfd *obfd, asection *osec)
> {
> @@ -599,8 +767,9 @@ gcore_copy_callback (bfd *obfd, asection *osec)
> bfd_section_vma (osec)));
> break;
> }
> - if (!bfd_set_section_contents (obfd, osec, memhunk.data (),
> - offset, size))
> +
> + if (!sparse_bfd_set_section_contents (obfd, osec, memhunk.data (),
> + offset, size))
> {
> warning (_("Failed to write corefile contents (%s)."),
> bfd_errmsg (bfd_get_error ()));
Best,
Lancelot.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-21 21:27 ` Lancelot SIX
@ 2024-03-21 23:14 ` Pedro Alves
2024-03-22 10:26 ` Lancelot SIX
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2024-03-21 23:14 UTC (permalink / raw)
To: Lancelot SIX, gdb-patches, John Baldwin
On 2024-03-21 21:27, Lancelot SIX wrote:
> I have a couple of small comments below.
Thanks!
>
> On 18/03/2024 17:43, Pedro Alves wrote:
>> diff --git a/gdb/gcore.c b/gdb/gcore.c
>> +
>> +/* Wrapper around bfd_set_section_contents that avoids writing
>> + all-zero blocks to disk, so we create a sparse core file.
>> + SKIP_ALIGN is a recursion helper -- if true, we'll skip aligning
>> + the file position to SPARSE_BLOCK_SIZE. */
>> +
>> +static bool
>> +sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
>> + const gdb_byte *data,
>> + size_t sec_offset,
>> + size_t size,
>> + bool skip_align = false)
>> +{
>> + /* Note, we don't have to have special handling for the case of the
>> + last memory region ending with zeros, because our caller always
>> + writes out the note section after the memory/load sections. If
>> + it didn't, we'd have to seek+write the last byte to make the file
>> + size correct. (Or add an ftruncate abstraction to bfd and call
>> + that.) */
>> +
>> + if (!skip_align)
>> + {
>> + /* Align the all-zero block search with SPARSE_BLOCK_SIZE, to
>> + better align with filesystem blocks. If we find we're
>> + misaligned, then write/skip the bytes needed to make us
>> + aligned. We do that with (one level) recursion. */
>> +
>> + /* We need to know the section's file offset on disk. We can
>> + only look at it after the bfd's 'output_has_begun' flag has
>> + been set, as bfd hasn't computed the file offsets
>> + otherwise. */
>> + if (!obfd->output_has_begun)
>> + {
>> + gdb_byte dummy = 0;
>> +
>> + /* A write forces BFD to compute the bfd's section file
>> + positions. Zero size works for that too. */
>> + if (!bfd_set_section_contents (obfd, osec, &dummy, 0, 0))
>> + return false;
>> +
>> + gdb_assert (obfd->output_has_begun);
>> + }
>> +
>> + /* How much we need to write/skip in order to find the next
>> + SPARSE_BLOCK_SIZE filepos-aligned block. */
>> + size_t align_remainder
>> + = (SPARSE_BLOCK_SIZE
>> + - (osec->filepos + sec_offset) % SPARSE_BLOCK_SIZE);
>> +
>> + /* How much we'll actually write in the recursion call. */
>> + size_t align_write_size = std::min (size, align_remainder);
>> +
>> + if (align_write_size != 0)
>
> I think at this point align_write_size can be SPARSE_BLOCK_SIZE (i.e. sec_offset lands at a SPARSE_BLOCK_SIZE boundary in the underlying filesystem). If that's the case, and data+sec_offset starts with block of 0s, you'll write it to disk needlessly.
Good find.
> Not a big deal, but I'd go for:
>
> if (align_write_size % SPARSE_BLOCK_SIZE != 0)
>
I wrote something different, just because I dislike repeating the modulo operation.
It forced me to come up with better variable names, but I think the result is clearer.
Also, align_write_size can only be zero if we have something to write. But if we have nothing
to write, this whole aligning logic isn't needed either. So I added an early size == 0 check,
which then removed the need for this indentation level here.
>> + {
>> + /* Recurse, skipping the alignment code. */
>> + if (!sparse_bfd_set_section_contents (obfd, osec, data,
>> + sec_offset,
>> + align_write_size, true))
>> + return false;
>> +
>> + /* Skip over what we've written, and proceed with
>> + assumes-aligned logic. */
>> + data += align_write_size;
>> + sec_offset += align_write_size;
>> + size -= align_write_size;
>> + }
>> + }
>> +
>> + size_t data_offset = 0;
>
> Just because that got me to think while reading, having the first part of the procedure update data/sec_offset/size and the second part of the procedure update data_offset seems a bit inconsistent.
>
> I would probably move the declaration of data_offset at the very begining of the procedure update it consistently:
>
> size_t data_offset = 0;
> if (!skip_align)
> {
> […]
> if (align_write_size % SPARSE_BLOCK_SIZE != 0)
> {
> […]
> data_offset += align_write_size;
> }
> }
> while (data_offset < size)
> […]
I did this, and then while stepping through the code to confirm it all works correctly, I noticed that it
leads to code that is a little harder to debug, as data_offset is no longer neatly aligned to 0x1000
while stepping through the "assume-aligned" main algorithm. But that may just be me, and I do see your
point, so I kept the change.
Here's the updated patch. I also applied John's suggestion to use a continue.
---- 8< ----
From 07d61478c8d02f593d8ab8bc0270eb0a90d535dd Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 21 Mar 2024 23:07:46 +0000
Subject: [PATCH v4] Teach GDB to generate sparse core files (PR
corefiles/31494)
This commit teaches GDB's gcore command to generate sparse core files
(if supported by the filesystem).
To create a sparse file, all you have to do is skip writing zeros to
the file, instead lseek'ing-ahead over them.
The sparse logic is applied when writing the memory sections, as
that's where the bulk of the data and the zeros are.
The commit also tweaks gdb.base/bigcore.exp to make it exercise
gdb-generated cores in addition to kernel-generated cores. We
couldn't do that before, because GDB's gcore on that test's program
would generate a multi-GB non-sparse core (16GB on my system).
After this commit, gdb.base/bigcore.exp generates, when testing with
GDB's gcore, a much smaller core file, roughly in line with what the
kernel produces:
real sizes:
$ du --hu testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
2.2M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
2.0M testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
apparent sizes:
$ du --hu --apparent-size testsuite/outputs/gdb.base/bigcore/bigcore.corefile.*
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.gdb
16G testsuite/outputs/gdb.base/bigcore/bigcore.corefile.kernel
Time to generate the core also goes down significantly. On my machine, I get:
when writing to an SSD, from 21.0s, down to 8.0s
when writing to an HDD, from 31.0s, down to 8.5s
The changes to gdb.base/bigcore.exp are smaller than they look at
first sight. It's basically mostly refactoring -- moving most of the
code to a new procedure which takes as argument who should dump the
core, and then calling the procedure twice. I purposely did not
modernize any of the refactored code in this patch.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
Reviewed-By: Lancelot Six <lancelot.six@amd.com>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
---
gdb/NEWS | 4 +
gdb/doc/gdb.texinfo | 3 +
gdb/gcore.c | 186 +++++++++++++++++++++-
gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
4 files changed, 323 insertions(+), 108 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index d8ac0bb06a7..d1d25e4c24d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,10 @@ disassemble
command will now give an error. Previously the 'b' flag would
always override the 'r' flag.
+gcore
+generate-core-file
+ GDB now generates sparse core files, on systems that support it.
+
maintenance info line-table
Add an EPILOGUE-BEGIN column to the output of the command. It indicates
if the line is considered the start of the epilgoue, and thus a point at
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f093ee269e2..9224829bd93 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -13867,6 +13867,9 @@ Produce a core dump of the inferior process. The optional argument
specified, the file name defaults to @file{core.@var{pid}}, where
@var{pid} is the inferior process ID.
+If supported by the filesystem where the core is written to,
+@value{GDBN} generates a sparse core dump file.
+
Note that this command is implemented only for some systems (as of
this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 7c12aa3a777..3d3973bfaba 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -39,10 +39,21 @@
#include "gdbsupport/byte-vector.h"
#include "gdbsupport/scope-exit.h"
+/* To generate sparse cores, we look at the data to write in chunks of
+ this size when considering whether to skip the write. Only if we
+ have a full block of this size with all zeros do we skip writing
+ it. A simpler algorithm that would try to skip all zeros would
+ result in potentially many more write/lseek syscalls, as normal
+ data is typically sprinkled with many small holes of zeros. Also,
+ it's much more efficient to memcmp a block of data against an
+ all-zero buffer than to check each and every data byte against zero
+ one by one. */
+#define SPARSE_BLOCK_SIZE 0x1000
+
/* The largest amount of memory to read from the target at once. We
must throttle it to limit the amount of memory used by GDB during
generate-core-file for programs with large resident data. */
-#define MAX_COPY_BYTES (1024 * 1024)
+#define MAX_COPY_BYTES (256 * SPARSE_BLOCK_SIZE)
static const char *default_gcore_target (void);
static enum bfd_architecture default_gcore_arch (void);
@@ -98,7 +109,12 @@ write_gcore_file_1 (bfd *obfd)
bfd_set_section_alignment (note_sec, 0);
bfd_set_section_size (note_sec, note_size);
- /* Now create the memory/load sections. */
+ /* Now create the memory/load sections. Note
+ gcore_memory_sections's sparse logic is assuming that we'll
+ always write something afterwards, which we do: just below, we
+ write the note section. So there's no need for an ftruncate-like
+ call to grow the file to the right size if the last memory
+ sections were zeros and we skipped writing them. */
if (gcore_memory_sections (obfd) == 0)
error (_("gcore: failed to get corefile memory sections from target."));
@@ -567,6 +583,167 @@ objfile_find_memory_regions (struct target_ops *self,
return 0;
}
+/* Check if we have a block full of zeros at DATA within the [DATA,
+ DATA+SIZE) buffer. Returns the size of the all-zero block found.
+ Returns at most the minimum between SIZE and SPARSE_BLOCK_SIZE. */
+
+static size_t
+get_all_zero_block_size (const gdb_byte *data, size_t size)
+{
+ size = std::min (size, (size_t) SPARSE_BLOCK_SIZE);
+
+ /* A memcmp of a whole block is much faster than a simple for loop.
+ This makes a big difference, as with a for loop, this code would
+ dominate the performance and result in doubling the time to
+ generate a core, at the time of writing. With an optimized
+ memcmp, this doesn't even show up in the perf trace. */
+ static const gdb_byte all_zero_block[SPARSE_BLOCK_SIZE] = {};
+ if (memcmp (data, all_zero_block, size) == 0)
+ return size;
+ return 0;
+}
+
+/* Basically a named-elements pair, used as return type of
+ find_next_all_zero_block. */
+
+struct offset_and_size
+{
+ size_t offset;
+ size_t size;
+};
+
+/* Find the next all-zero block at DATA+OFFSET within the [DATA,
+ DATA+SIZE) buffer. Returns the offset and the size of the all-zero
+ block if found, or zero if not found. */
+
+static offset_and_size
+find_next_all_zero_block (const gdb_byte *data, size_t offset, size_t size)
+{
+ for (; offset < size; offset += SPARSE_BLOCK_SIZE)
+ {
+ size_t zero_block_size
+ = get_all_zero_block_size (data + offset, size - offset);
+ if (zero_block_size != 0)
+ return {offset, zero_block_size};
+ }
+ return {0, 0};
+}
+
+/* Wrapper around bfd_set_section_contents that avoids writing
+ all-zero blocks to disk, so we create a sparse core file.
+ SKIP_ALIGN is a recursion helper -- if true, we'll skip aligning
+ the file position to SPARSE_BLOCK_SIZE. */
+
+static bool
+sparse_bfd_set_section_contents (bfd *obfd, asection *osec,
+ const gdb_byte *data,
+ size_t sec_offset,
+ size_t size,
+ bool skip_align = false)
+{
+ /* Note, we don't have to have special handling for the case of the
+ last memory region ending with zeros, because our caller always
+ writes out the note section after the memory/load sections. If
+ it didn't, we'd have to seek+write the last byte to make the file
+ size correct. (Or add an ftruncate abstraction to bfd and call
+ that.) */
+
+ if (size == 0)
+ return true;
+
+ size_t data_offset = 0;
+
+ if (!skip_align)
+ {
+ /* Align the all-zero block search with SPARSE_BLOCK_SIZE, to
+ better align with filesystem blocks. If we find we're
+ misaligned, then write/skip the bytes needed to make us
+ aligned. We do that with (one level) recursion. */
+
+ /* We need to know the section's file offset on disk. We can
+ only look at it after the bfd's 'output_has_begun' flag has
+ been set, as bfd hasn't computed the file offsets
+ otherwise. */
+ if (!obfd->output_has_begun)
+ {
+ gdb_byte dummy = 0;
+
+ /* A write forces BFD to compute the bfd's section file
+ positions. Zero size works for that too. */
+ if (!bfd_set_section_contents (obfd, osec, &dummy, 0, 0))
+ return false;
+
+ gdb_assert (obfd->output_has_begun);
+ }
+
+ /* How much after the last aligned offset are we writing at. */
+ size_t aligned_offset_remainder
+ = (osec->filepos + sec_offset) % SPARSE_BLOCK_SIZE;
+
+ /* Do we need to align? */
+ if (aligned_offset_remainder != 0)
+ {
+ /* How much we need to advance in order to find the next
+ SPARSE_BLOCK_SIZE filepos-aligned block. */
+ size_t distance_to_next_aligned
+ = SPARSE_BLOCK_SIZE - aligned_offset_remainder;
+
+ /* How much we'll actually write in the recursion call. The
+ caller may want us to write fewer bytes than
+ DISTANCE_TO_NEXT_ALIGNED. */
+ size_t align_write_size = std::min (size, distance_to_next_aligned);
+
+ /* Recurse, skipping the alignment code. */
+ if (!sparse_bfd_set_section_contents (obfd, osec, data,
+ sec_offset,
+ align_write_size, true))
+ return false;
+
+ /* Skip over what we've written, and proceed with
+ assumes-aligned logic. */
+ data_offset += align_write_size;
+ }
+ }
+
+ while (data_offset < size)
+ {
+ size_t all_zero_block_size
+ = get_all_zero_block_size (data + data_offset, size - data_offset);
+ if (all_zero_block_size != 0)
+ {
+ /* Skip writing all-zero blocks. */
+ data_offset += all_zero_block_size;
+ continue;
+ }
+
+ /* We have some non-zero data to write to file. Find the next
+ all-zero block within the data, and only write up to it. */
+
+ offset_and_size next_all_zero_block
+ = find_next_all_zero_block (data,
+ data_offset + SPARSE_BLOCK_SIZE,
+ size);
+ size_t next_data_offset = (next_all_zero_block.offset == 0
+ ? size
+ : next_all_zero_block.offset);
+
+ if (!bfd_set_section_contents (obfd, osec, data + data_offset,
+ sec_offset + data_offset,
+ next_data_offset - data_offset))
+ return false;
+
+ data_offset = next_data_offset;
+
+ /* If we already know we have an all-zero block at the next
+ offset, we can skip calling get_all_zero_block_size for
+ it again. */
+ if (next_all_zero_block.offset != 0)
+ data_offset += next_all_zero_block.size;
+ }
+
+ return true;
+}
+
static void
gcore_copy_callback (bfd *obfd, asection *osec)
{
@@ -599,8 +776,9 @@ gcore_copy_callback (bfd *obfd, asection *osec)
bfd_section_vma (osec)));
break;
}
- if (!bfd_set_section_contents (obfd, osec, memhunk.data (),
- offset, size))
+
+ if (!sparse_bfd_set_section_contents (obfd, osec, memhunk.data (),
+ offset, size))
{
warning (_("Failed to write corefile contents (%s)."),
bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
index 3f9ae48abf2..6c64d402502 100644
--- a/gdb/testsuite/gdb.base/bigcore.exp
+++ b/gdb/testsuite/gdb.base/bigcore.exp
@@ -43,23 +43,6 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
return -1
}
-# Run GDB on the bigcore program up-to where it will dump core.
-
-clean_restart ${binfile}
-gdb_test_no_output "set print sevenbit-strings"
-gdb_test_no_output "set width 0"
-
-# Get the core into the output directory.
-set_inferior_cwd_to_output_dir
-
-if {![runto_main]} {
- return 0
-}
-set print_core_line [gdb_get_line_number "Dump core"]
-gdb_test "tbreak $print_core_line"
-gdb_test continue ".*print_string.*"
-gdb_test next ".*0 = 0.*"
-
# Traverse part of bigcore's linked list of memory chunks (forward or
# backward), saving each chunk's address.
@@ -92,92 +75,11 @@ proc extract_heap { dir } {
}
return $heap
}
-set next_heap [extract_heap next]
-set prev_heap [extract_heap prev]
-
-# Save the total allocated size within GDB so that we can check
-# the core size later.
-gdb_test_no_output "set \$bytes_allocated = bytes_allocated" "save heap size"
-
-# Now create a core dump
-
-# Rename the core file to "TESTFILE.corefile" rather than just "core",
-# to avoid problems with sys admin types that like to regularly prune
-# all files named "core" from the system.
-
-# Some systems append "core" to the name of the program; others append
-# the name of the program to "core"; still others (like Linux, as of
-# May 2003) create cores named "core.PID".
-
-# Save the process ID. Some systems dump the core into core.PID.
-set inferior_pid [get_inferior_pid]
-
-# Dump core using SIGABRT
-set oldtimeout $timeout
-set timeout 600
-gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
-set timeout $oldtimeout
-
-# Find the corefile.
-set file [find_core_file $inferior_pid]
-if { $file != "" } {
- remote_exec build "mv $file $corefile"
-} else {
- untested "can't generate a core file"
- return 0
-}
-# Check that the corefile is plausibly large enough. We're trying to
-# detect the case where the operating system has truncated the file
-# just before signed wraparound. TCL, unfortunately, has a similar
-# problem - so use catch. It can handle the "bad" size but not
-# necessarily the "good" one. And we must use GDB for the comparison,
-# similarly.
-
-if {[catch {file size $corefile} core_size] == 0} {
- set core_ok 0
- gdb_test_multiple "print \$bytes_allocated < $core_size" "check core size" {
- -re " = 1\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 1
- }
- -re " = 0\r\n$gdb_prompt $" {
- pass "check core size"
- set core_ok 0
- }
- }
-} {
- # Probably failed due to the TCL build having problems with very
- # large values. Since GDB uses a 64-bit off_t (when possible) it
- # shouldn't have this problem. Assume that things are going to
- # work. Without this assumption the test is skiped on systems
- # (such as i386 GNU/Linux with patched kernel) which do pass.
- pass "check core size"
- set core_ok 1
-}
-if {! $core_ok} {
- untested "check core size (system does not support large corefiles)"
- return 0
-}
-
-# Now load up that core file
-
-set test "load corefile"
-gdb_test_multiple "core $corefile" "$test" {
- -re "A program is being debugged already. Kill it. .y or n. " {
- send_gdb "y\n"
- exp_continue
- }
- -re "Core was generated by.*$gdb_prompt $" {
- pass "$test"
- }
-}
-
-# Finally, re-traverse bigcore's linked list, checking each chunk's
-# address against the executable. Don't use gdb_test_multiple as want
-# only one pass/fail. Don't use exp_continue as the regular
-# expression involving $heap needs to be re-evaluated for each new
-# response.
+# Re-traverse bigcore's linked list, checking each chunk's address
+# against the executable. Don't use gdb_test_multiple as want only
+# one pass/fail. Don't use exp_continue as the regular expression
+# involving $heap needs to be re-evaluated for each new response.
proc check_heap { dir heap } {
global gdb_prompt
@@ -208,5 +110,133 @@ proc check_heap { dir heap } {
}
}
-check_heap next $next_heap
-check_heap prev $prev_heap
+# The bulk of the testcase. DUMPER indicates who is supposed to dump
+# the core. It can be either "kernel", or "gdb".
+proc test {dumper} {
+ global binfile timeout corefile gdb_prompt
+
+ # Run GDB on the bigcore program up-to where it will dump core.
+
+ clean_restart ${binfile}
+ gdb_test_no_output "set print sevenbit-strings"
+ gdb_test_no_output "set width 0"
+
+ # Get the core into the output directory.
+ set_inferior_cwd_to_output_dir
+
+ if {![runto_main]} {
+ return 0
+ }
+ set print_core_line [gdb_get_line_number "Dump core"]
+ gdb_test "tbreak $print_core_line"
+ gdb_test continue ".*print_string.*"
+ gdb_test next ".*0 = 0.*"
+
+ set next_heap [extract_heap next]
+ set prev_heap [extract_heap prev]
+
+ # Save the total allocated size within GDB so that we can check
+ # the core size later.
+ gdb_test_no_output "set \$bytes_allocated = bytes_allocated" \
+ "save heap size"
+
+ # Now create a core dump.
+
+ if {$dumper == "kernel"} {
+ # Rename the core file to "TESTFILE.corefile.$dumper" rather
+ # than just "core", to avoid problems with sys admin types
+ # that like to regularly prune all files named "core" from the
+ # system.
+
+ # Some systems append "core" to the name of the program;
+ # others append the name of the program to "core"; still
+ # others (like Linux, as of May 2003) create cores named
+ # "core.PID".
+
+ # Save the process ID. Some systems dump the core into
+ # core.PID.
+ set inferior_pid [get_inferior_pid]
+
+ # Dump core using SIGABRT.
+ set oldtimeout $timeout
+ set timeout 600
+ gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
+ set timeout $oldtimeout
+
+ # Find the corefile.
+ set file [find_core_file $inferior_pid]
+ if { $file != "" } {
+ remote_exec build "mv $file $corefile.$dumper"
+ } else {
+ untested "can't generate a core file"
+ return 0
+ }
+ } elseif {$dumper == "gdb"} {
+ gdb_gcore_cmd "$corefile.$dumper" "gcore corefile"
+ } else {
+ error "unhandled dumper: $dumper"
+ }
+
+ # Check that the corefile is plausibly large enough. We're trying
+ # to detect the case where the operating system has truncated the
+ # file just before signed wraparound. TCL, unfortunately, has a
+ # similar problem - so use catch. It can handle the "bad" size
+ # but not necessarily the "good" one. And we must use GDB for the
+ # comparison, similarly.
+
+ if {[catch {file size $corefile.$dumper} core_size] == 0} {
+ set core_ok 0
+ gdb_test_multiple "print \$bytes_allocated < $core_size" \
+ "check core size" {
+ -re " = 1\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 1
+ }
+ -re " = 0\r\n$gdb_prompt $" {
+ pass "check core size"
+ set core_ok 0
+ }
+ }
+ } {
+ # Probably failed due to the TCL build having problems with
+ # very large values. Since GDB uses a 64-bit off_t (when
+ # possible) it shouldn't have this problem. Assume that
+ # things are going to work. Without this assumption the test
+ # is skiped on systems (such as i386 GNU/Linux with patched
+ # kernel) which do pass.
+ pass "check core size"
+ set core_ok 1
+ }
+ if {! $core_ok} {
+ untested "check core size (system does not support large corefiles)"
+ return 0
+ }
+
+ # Now load up that core file.
+
+ set test "load corefile"
+ gdb_test_multiple "core $corefile.$dumper" "$test" {
+ -re "A program is being debugged already. Kill it. .y or n. " {
+ send_gdb "y\n"
+ exp_continue
+ }
+ -re "Core was generated by.*$gdb_prompt $" {
+ pass "$test"
+ }
+ }
+
+ # Finally, re-traverse bigcore's linked list, checking each
+ # chunk's address against the executable.
+
+ check_heap next $next_heap
+ check_heap prev $prev_heap
+}
+
+foreach_with_prefix dumper {kernel gdb} {
+ # GDB's gcore is too slow when testing with the extended-gdbserver
+ # board, since it requires reading all the inferior memory.
+ if {$dumper == "gdb" && [target_info gdb_protocol] != ""} {
+ continue
+ }
+ test $dumper
+}
base-commit: 9bec569fda7c76849cf3eb0e4a525f627d25f980
--
2.43.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-21 23:14 ` [PATCH v4] " Pedro Alves
@ 2024-03-22 10:26 ` Lancelot SIX
2024-03-22 12:33 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Lancelot SIX @ 2024-03-22 10:26 UTC (permalink / raw)
To: Pedro Alves, gdb-patches, John Baldwin
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31494
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>
> Change-Id: I2554a6a4a72d8c199ce31f176e0ead0c0c76cff1
> ---
> gdb/NEWS | 4 +
> gdb/doc/gdb.texinfo | 3 +
> gdb/gcore.c | 186 +++++++++++++++++++++-
> gdb/testsuite/gdb.base/bigcore.exp | 238 ++++++++++++++++-------------
> 4 files changed, 323 insertions(+), 108 deletions(-)
>
I just had a quick look at the changes, and that looks good to me.
Thanks for doing this!
Lancelot.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Teach GDB to generate sparse core files (PR corefiles/31494)
2024-03-22 10:26 ` Lancelot SIX
@ 2024-03-22 12:33 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2024-03-22 12:33 UTC (permalink / raw)
To: Lancelot SIX, gdb-patches, John Baldwin
On 2024-03-22 10:26, Lancelot SIX wrote:
> I just had a quick look at the changes, and that looks good to me.
>
> Thanks for doing this!
>
Thanks! I've merged this now.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-22 12:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-15 18:27 [PATCH] Teach GDB to generate sparse core files (PR corefiles/31494) Pedro Alves
2024-03-15 18:40 ` Eli Zaretskii
2024-03-18 13:29 ` [PATCH v2] " Pedro Alves
2024-03-18 17:43 ` [PATCH v3] " Pedro Alves
2024-03-19 16:41 ` John Baldwin
2024-03-21 21:27 ` Lancelot SIX
2024-03-21 23:14 ` [PATCH v4] " Pedro Alves
2024-03-22 10:26 ` Lancelot SIX
2024-03-22 12:33 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox