Re: [PATCH v3] Teach GDB to generate sparse core files (PR corefiles/31494)

[John Baldwin <jhb@FreeBSD.org>]

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