Re: [PATCH] gcore: handle known-all-zeroes hole mappings

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> This commit improves core generation on Linux.  The issue this
> addresses became apparent when dumping core of HIP programs on AMD
> GPUs, but it's strictly a CPU-side issue.
>
> The AMD GPU runtime used by HIP (and other languages) programs creates
> these big mappings in every program, the size of GPU VRAM.  E.g., on
> gfx942 with 256GB VRAM, we end up with 256GB of such mappings:
>
>   (gdb) info proc mappings
>   ...
>   0x00007fbfe7000000 0x00007fc7e7000000 0x800000000        0x0                ---p
>   0x00007fc7e7200000 0x00007fcfe7200000 0x800000000        0x0                ---p
>   0x00007fcfe7400000 0x00007fd7e7400000 0x800000000        0x0                ---p
>   0x00007fd7e7600000 0x00007fdfe7600000 0x800000000        0x0                ---p
>   0x00007fdfe7800000 0x00007fe7e7800000 0x800000000        0x0                ---p
>   0x00007fe7e7a00000 0x00007fefe7a00000 0x800000000        0x0                ---p
>   0x00007fefe7c00000 0x00007ff7e7c00000 0x800000000        0x0                ---p
>   ...
>
> They show up like this in /proc/pid/smaps
>
>   ...
>   7fbfe7000000-7fc7e7000000 ---p 00000000 00:00 0
>   Size:           33554432 kB
>   KernelPageSize:        4 kB
>   MMUPageSize:           4 kB
>   Rss:                   0 kB
>   Pss:                   0 kB
>   Pss_Dirty:             0 kB
>   Shared_Clean:          0 kB
>   Shared_Dirty:          0 kB
>   Private_Clean:         0 kB
>   Private_Dirty:         0 kB
>   Referenced:            0 kB
>   Anonymous:             0 kB
>   KSM:                   0 kB
>   LazyFree:              0 kB
>   AnonHugePages:         0 kB
>   ShmemPmdMapped:        0 kB
>   FilePmdMapped:         0 kB
>   Shared_Hugetlb:        0 kB
>   Private_Hugetlb:       0 kB
>   Swap:                  0 kB
>   SwapPss:               0 kB
>   Locked:                0 kB
>   THPeligible:           0
>   ProtectionKey:         0
>   VmFlags: mr mw me nr sd
>   ...
>
> Note the mappings in question have no backing data: the Rss: and Swap:
> fields are zero.  The mappings exist only to reserve VMAs on the CPU
> side.
>
> When the Linux kernel itself dumps core for such a process, here's
> what objdump -h on the core shows for such mappings:
>
>  Idx Name          Size      VMA               LMA               File off  Algn
>  ...
>  39 load17        800000000  000070aeb3200000  0000000000000000  17555000  2**12
>                   ALLOC, READONLY
>  40 load18        800000000  000070b6b3400000  0000000000000000  17555000  2**12
>                   ALLOC, READONLY
>  ...
>
> GDB's gcore, not knowing the mappings contain only zeroes, reads all
> their contents from inferior memory, all 256GB of it.  On gfx942, that
> makes the gcore command take around 1 minute when dumping a small HIP
> program.
>
> GDB's gcore currently generates load segments of these mappings like
> this:
>
>  Idx Name          Size      VMA               LMA               File off  Algn
>  ...
>  40 load17        800000000  00007fcfe7400000  0000000000000000  101e0a7bd0  2**0
>
> 		  CONTENTS, ALLOC, LOAD, READONLY
>
>  41 load18        800000000  00007fd7e7600000  0000000000000000  181e0a7bd0  2**0
>
> 		  CONTENTS, ALLOC, LOAD, READONLY
>  ...
>
> The fact that GDB makes CONTENTS/LOAD segments makes it so that the
> resulting core files are much larger than what the Linux kernel
> produces (note "File off" column), even though it's mostly apparent
> size as gdb knows how to produce sparse cores:
>
>  $ ls -als --hu core/*
>  440M -rw------- 1 pedalves pedalves 474M Mar 24 08:14 core/kernel.core
>   51M -rw-rw-r-- 1 pedalves pedalves 257G Mar 24 08:16 core/gdb-before.core
>  ^^^^ real                           ^^^^ apparent
>
> This commit teaches linux-tdep.c to identify anonymous mappings that
> have no backing data from /proc/pid/smaps (which we already parse), so
> that the gcore code can skip reading inferior memory for them, and
> their load segments can be emitted with no CONTENTS/LOAD.
>
> The skip-reading-inferior-memory part speeds up core dumping of small
> HIP inferiors on gfx942 GPUs by a large factor.  E.g.:
>
> With:
>
>  $ time rocgdb --batch -q small-test-program \
>       -ex "with breakpoint pending on -- b gpu_code" \
>       -ex "r" \
>       -ex "gcore" \
>       -ex "k"
>
> On gfx942, before the patch:
>
>  real    1m6.518s
>  user    0m5.984s
>  sys     1m0.218s
>
> On gfx942, after the patch:
>
>  real    0m4.456s
>  user    0m2.219s
>  sys     0m1.829s
>
> And the fact that we no longer emit segments with CONTENTS/ALLOW makes

typo? CONTENTS/LOAD maybe?

> the resulting gdb-generated cores's apparent size be much smaller,
> closer to kernel-generated cores's:

I think cores' would be correct here.  I'd usually write "core file's"
specifically to avoid worrying about this though!

>
>  $ ls -als --hu core/*
>  440M -rw------- 1 pedalves pedalves 474M Mar 24 08:14 core/kernel.core
>   51M -rw-rw-r-- 1 pedalves pedalves 257G Mar 24 08:16 core/gdb-before.core
>   52M -rw-rw-r-- 1 pedalves pedalves 642M Mar 24 09:12 core/gdb-after.core
>
> This commit also adds a new testcase that exercises both the scenario
> in question (without relying on the HIP runtime), and the converse of
> making sure that we don't skip dumping anonymous private PROT_NONE
> mappings with backing data, by mistake.
>
> Change-Id: I2cf21409af36266094bcff5614770605fab4030e
> commit-id: d3d471d8
> ---
>  gdb/find-memory-region.h                      |   5 +-
>  gdb/gcore.c                                   |  28 ++--
>  gdb/linux-tdep.c                              |  85 +++++++++--
>  .../gdb.base/gcore-anon-priv-protnone.c       |  60 ++++++++
>  .../gdb.base/gcore-anon-priv-protnone.exp     | 133 ++++++++++++++++++
>  5 files changed, 288 insertions(+), 23 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/gcore-anon-priv-protnone.c
>  create mode 100644 gdb/testsuite/gdb.base/gcore-anon-priv-protnone.exp
>
> diff --git a/gdb/find-memory-region.h b/gdb/find-memory-region.h
> index 23776caebb2..85f4a4acd0f 100644
> --- a/gdb/find-memory-region.h
> +++ b/gdb/find-memory-region.h
> @@ -27,11 +27,14 @@
>     MEMORY_TAGGED is true if the memory region contains memory tags, false
>     otherwise.
>  
> +   HOLE is true if the memory region is known to be all zeroes, false
> +   otherwise.
> +
>     Return true on success, false otherwise.  */
>  
>  using find_memory_region_ftype
>    = gdb::function_view<bool (CORE_ADDR addr, unsigned long size, bool read,
>  			     bool write, bool exec, bool modified,
> -			     bool memory_tagged)>;
> +			     bool memory_tagged, bool hole)>;

The find_memory_region_ftype is used in:

  - gdb/fbsd-nat.c -- fbsd_nat_target::find_memory_regions
  - gdb/netbsd-nat.c -- nbsd_nat_target::find_memory_regions
  - gdb/gnu-nat.c -- gnu_nat_target::find_memory_regions

None of these have been updated in this commit, but I think they need to
be?  I suspect they probably all just need to pass `false` for the HOLE
argument.  If I've understood this correctly, then they would at least
be no worse off by doing that, right?

>  
>  #endif /* GDB_FIND_MEMORY_REGION_H */
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 149ade2a08f..5c0fe325639 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -403,10 +403,13 @@ make_output_phdrs (bfd *obfd, asection *osec)
>  static bool
>  gcore_create_callback (CORE_ADDR vaddr, unsigned long size, bool read,
>  		       bool write, bool exec, bool modified,
> -		       bool memory_tagged, bfd *obfd)
> +		       bool memory_tagged, bool hole, bfd *obfd)

We should probably update the function comment to explain the new HOLE
argument.

>  {
>    asection *osec;
> -  flagword flags = SEC_ALLOC | SEC_HAS_CONTENTS | SEC_LOAD;
> +  flagword flags = SEC_ALLOC;
> +
> +  if (!hole)
> +    flags |= SEC_HAS_CONTENTS | SEC_LOAD;
>  
>    /* If the memory segment has no permissions set, ignore it, otherwise
>       when we later try to access it for read/write, we'll get an error
> @@ -421,7 +424,7 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, bool read,
>        return true;
>      }
>  
> -  if (!write && !modified && !solib_keep_data_in_core (vaddr, size))
> +  if (!hole && !write && !modified && !solib_keep_data_in_core (vaddr, size))
>      {
>        /* See if this region of memory lies inside a known file on disk.
>  	 If so, we can avoid copying its contents by clearing SEC_LOAD.  */
> @@ -493,7 +496,7 @@ static bool
>  gcore_create_memtag_section_callback (CORE_ADDR vaddr, unsigned long size,
>  				      bool read, bool write, bool exec,
>  				      bool modified, bool memory_tagged,
> -				      bfd *obfd)
> +				      bool hole, bfd *obfd)
>  {
>    /* Are there memory tags in this particular memory map entry?  */
>    if (!memory_tagged)
> @@ -550,7 +553,8 @@ objfile_find_memory_regions (struct target_ops *self,
>  			     (flags & SEC_READONLY) == 0, /* Writable.  */
>  			     (flags & SEC_CODE) != 0, /* Executable.  */
>  			     true, /* MODIFIED is unknown, pass it as true.  */
> -			     false /* No memory tags in the object file.  */);
> +			     false, /* No memory tags in the object file.  */
> +			     false  /* Not known to be an all-zeroes hole.  */);
>  	    if (!ret)
>  	      return ret;
>  	  }
> @@ -563,7 +567,8 @@ objfile_find_memory_regions (struct target_ops *self,
>  		true,  /* Stack section will be writable.  */
>  		false, /* Stack section will not be executable.  */
>  		true,  /* Stack section will be modified.  */
> -		false  /* No memory tags in the object file.  */))
> +		false, /* No memory tags in the object file.  */
> +		false  /* Not known to be an all-zeroes hole.  */))
>      return false;
>  
>    /* Make a heap segment.  */
> @@ -574,7 +579,8 @@ objfile_find_memory_regions (struct target_ops *self,
>  		true,  /* Heap section will be writable.  */
>  		false, /* Heap section will not be executable.  */
>  		true,  /* Heap section will be modified.  */
> -		false  /* No memory tags in the object file.  */))
> +		false, /* No memory tags in the object file.  */
> +		false  /* Not known to be an all-zeroes hole.  */))
>      return false;
>  
>    return true;
> @@ -845,10 +851,10 @@ static bool
>  gcore_memory_sections (bfd *obfd)
>  {
>    auto cb = [obfd] (CORE_ADDR vaddr, unsigned long size, bool read, bool write,
> -		    bool exec, bool modified, bool memory_tagged)
> +		    bool exec, bool modified, bool memory_tagged, bool hole)
>      {
>        return gcore_create_callback (vaddr, size, read, write, exec, modified,
> -				    memory_tagged, obfd);
> +				    memory_tagged, hole, obfd);
>      };
>  
>    /* Try gdbarch method first, then fall back to target method.  */
> @@ -862,11 +868,11 @@ gcore_memory_sections (bfd *obfd)
>  
>    auto cb_memtag = [obfd] (CORE_ADDR vaddr, unsigned long size, bool read,
>  			   bool write, bool exec, bool modified,
> -			   bool memory_tagged)
> +			   bool memory_tagged, bool hole)
>      {
>        return gcore_create_memtag_section_callback (vaddr, size, read, write,
>  						   exec, modified,
> -						   memory_tagged, obfd);
> +						   memory_tagged, hole, obfd);
>      };
>  
>    /* Take care of dumping memory tags, if there are any.  */
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 4bc5a2b6948..1abb9fba067 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -121,6 +121,9 @@ struct smaps_data
>  
>    ULONGEST inode;
>    ULONGEST offset;
> +
> +  ULONGEST rss;
> +  ULONGEST swap;
>  };
>  
>  /* Whether to take the /proc/PID/coredump_filter into account when
> @@ -1425,12 +1428,46 @@ linux_core_xfer_siginfo (struct gdbarch *gdbarch, struct bfd &cbfd,
>  }
>  
>  using linux_find_memory_region_ftype
> -  = gdb::function_view<bool (ULONGEST, ULONGEST, ULONGEST, bool, bool, bool,
> -			     bool, bool, const std::string &)>;
> +  = gdb::function_view<bool (ULONGEST /* vaddr */,
> +			     ULONGEST /* size */,
> +			     ULONGEST /* offset */,
> +			     bool /* read */,
> +			     bool /* write */,
> +			     bool /* exec */,
> +			     bool /* modified */,
> +			     bool /* memory_tagged */,
> +			     bool /* hole */,
> +			     const std::string & /* filename */)>;
>  
>  typedef bool linux_dump_mapping_p_ftype (filter_flags filterflags,
>  					 const smaps_data &map);
>  
> +/* Parse a KEY value out of a /proc/pid/smaps line.  KEYWORD is the
> +   keyword that was extracted out of the LINE we're considering.
> +   MAPS_FILENAME is the /proc/pid/smaps filename.  The result is
> +   written to *VALUE.  Returns true if LINE is a line for KEY, false
> +   otherwise.  */
> +
> +static bool
> +parse_smaps_key_value (const char *keyword, const char *line,
> +		       const char *key,
> +		       const std::string &maps_filename,
> +		       ULONGEST *value)
> +{
> +  if (!streq (keyword, key))
> +    return false;
> +
> +  const char *startptr = skip_spaces (line + strlen (key));
> +  const char *endptr;
> +  *value = strtoulst (startptr, &endptr, 0);
> +  if (endptr == startptr)
> +    {
> +      warning (_("Error parsing {s,}maps file '%s' number"),
> +	       maps_filename.c_str ());
> +    }
> +  return true;
> +}
> +
>  /* Helper function to parse the contents of /proc/<pid>/smaps into a data
>     structure, for easy access.
>  
> @@ -1456,6 +1493,8 @@ parse_smaps_data (const char *data,
>        int has_anonymous = 0;
>        int mapping_anon_p;
>        int mapping_file_p;
> +      ULONGEST rss = -1;
> +      ULONGEST swap = -1;
>  
>        memset (&v, 0, sizeof (v));
>        struct mapping m = read_mapping (line);
> @@ -1513,6 +1552,16 @@ parse_smaps_data (const char *data,
>  	  else if (streq (keyword, "VmFlags:"))
>  	    decode_vmflags (line, &v);
>  
> +	  if (parse_smaps_key_value (keyword, line, "Rss:",
> +				     maps_filename,
> +				     &rss))
> +	    continue;
> +
> +	  if (parse_smaps_key_value (keyword, line, "Swap:",
> +				     maps_filename,
> +				     &swap))
> +	    continue;
> +
>  	  if (streq (keyword, "AnonHugePages:")
>  	      || streq (keyword, "Anonymous:"))
>  	    {
> @@ -1562,6 +1611,8 @@ parse_smaps_data (const char *data,
>  	map.mapping_file_p = mapping_file_p? true : false;
>  	map.offset = m.offset;
>  	map.inode = m.inode;
> +	map.rss = rss;
> +	map.swap = swap;
>  
>  	smaps.emplace_back (map);
>      }
> @@ -1692,12 +1743,23 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>    for (const struct smaps_data &map : smaps)
>      {
>        /* Invoke the callback function to create the corefile segment.  */
> -      if (should_dump_mapping_p (filterflags, map)
> -	  && !func (map.start_address, map.end_address - map.start_address,
> -		    map.offset, map.read, map.write, map.exec,
> -		    /* MODIFIED is true because we want to dump the mapping.  */
> -		    true, map.vmflags.memory_tagging != 0, map.filename))
> -	return false;
> +      if (should_dump_mapping_p (filterflags, map))
> +	{
> +	  /* If this an anonymous PROT_NONE private mapping, check if

missing word: "If this IS an anonymous ..."

> +	     the mapping has any physical memory backing.  If not,
> +	     then we know that reading it would just yield zeroes, so
> +	     we can later skip reading it.  */
> +	  bool hole = (map.mapping_anon_p && map.priv
> +		       && !map.read && !map.write && !map.exec
> +		       && map.rss == 0 && map.swap == 0);
> +	  if (!func (map.start_address, map.end_address - map.start_address,
> +		     map.offset, map.read, map.write, map.exec,
> +		     /* MODIFIED is true because we want to dump the
> +			mapping.  */
> +		     true, map.vmflags.memory_tagging != 0, hole,
> +		     map.filename))
> +	    return false;
> +	}
>      }
>  
>    return true;
> @@ -1712,8 +1774,9 @@ linux_find_memory_regions (struct gdbarch *gdbarch,
>  {
>    auto cb = [&] (ULONGEST vaddr, ULONGEST size, ULONGEST offset, bool read,
>  		 bool write, bool exec, bool modified, bool memory_tagged,
> -		 const std::string &filename)
> -    { return func (vaddr, size, read, write, exec, modified, memory_tagged); };
> +		 bool hole, const std::string &filename)
> +    { return func (vaddr, size, read, write, exec, modified, memory_tagged,
> +		   hole); };
>  
>    return linux_find_memory_regions_full (gdbarch, dump_mapping_p, cb);
>  }
> @@ -1741,7 +1804,7 @@ linux_make_mappings_corefile_notes (struct gdbarch *gdbarch, bfd *obfd,
>  
>    auto cb = [&] (ULONGEST vaddr, ULONGEST size, ULONGEST offset, bool read,
>  		 bool write, bool exec, bool modified, bool memory_tagged,
> -		 const std::string &filename)
> +		 bool hole, const std::string &filename)
>      {
>        gdb_assert (filename.length () > 0);
>  
> diff --git a/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.c b/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.c
> new file mode 100644
> index 00000000000..38cd5da4fb0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.c
> @@ -0,0 +1,60 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2026 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +void
> +done ()
> +{
> +}
> +
> +void *mmapped_data;
> +
> +int
> +main ()
> +{
> +  /* 10 pages on most systems.  */
> +  size_t sz = 4096 * 10;
> +
> +  /* Allocate anonymous memory.  */
> +  mmapped_data = mmap (NULL, sz, PROT_READ | PROT_WRITE,
> +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +  if (mmapped_data == MAP_FAILED)
> +    {
> +      perror ("mmap");
> +      return 1;
> +    }
> +
> +#ifdef FILL_WITH_DATA
> +  /* Fill with a recognizable pattern.  */
> +  memset (mmapped_data, 0xab, sz);
> +#endif
> +
> +  /* Remove all permissions.  */
> +  if (mprotect (mmapped_data, sz, PROT_NONE) == -1)
> +    {
> +      perror ("mprotect");
> +      return 1;
> +    }
> +
> +  done ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.exp b/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.exp
> new file mode 100644
> index 00000000000..d6c32b20cf4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-anon-priv-protnone.exp
> @@ -0,0 +1,133 @@
> +# Copyright 2026 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Make sure that:
> +#
> +# data mode - gcore dumps 'PROT_NONE anonymous private mappings that
> +# have resident data' as segments with data.
> +#
> +# nodata mode - gcore dumps 'PROT_NONE anonymous private mappings that
> +# have no resident data' as segments with no data (not even zeroes).
> +#
> +# Also make sure that GDB is able to read back the produced cores and
> +# read the contents of the mappings.  In nodata mode, GDB should read
> +# back zeroes.
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +     return
> +}

This builds the executable and does a 'clean_restart', but each call to
TEST also builds the executable.  This might serve as an early exit in
the case where the executable doesn't build, in which case you can just
use build_executable.  But honestly, I don't think that's necessary, I'd
just drop this completely.  Worse case, we make multiple (2) calls to
TEST, and in each case the executable fails to build and we bail out.

> +
> +# MODE is either data or nodata.
> +proc test {mode} {
> +    global srcfile binfile
> +
> +    set options {debug}
> +    if { $mode == "data" } {
> +	lappend options "additional_flags=-DFILL_WITH_DATA"
> +    }
> +
> +    if { [build_executable "failed to prepare" ${binfile}-$mode ${srcfile} $options] } {
> +	return
> +    }
> +
> +    clean_restart
> +    gdb_load $binfile-$mode

I think you can replace the build_executable, clean_restart, and
gdb_load with a single prepare_for_testing call.

I think everything I've pointed out is pretty minor.  If you're happy to
fix it then:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew