From: Andrew Burgess <aburgess@redhat.com>
Date: Wed, 7 May 2025 09:33:09 +0000 (+0100)
Subject: gdb: always call should_dump_mapping_p during core file creation
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dcbcfc9a520750a489cde5cef50d6e1557b77623;p=thirdparty%2Fbinutils-gdb.git

gdb: always call should_dump_mapping_p during core file creation

This commit moves the logic for whether should_dump_mapping_p is
called out of linux_find_memory_regions_full and pushes it down into
the two callback functions that are used as the should_dump_mapping_p
callback; `dump_mapping_p` and `dump_note_entry_p`.

Older Linux kernels don't make the 'Anonymous' information available
in the smaps file, and currently, GDB handles this by not calling the
should_dump_mapping_p callback in linux_find_memory_regions_full,
instead the answer is hard-coded to true.

This is (maybe) fine for dump_mapping_p, but for dump_note_entry_p,
this choice makes little sense.  The dump_note_entry_p function
doesn't even use the anonymous mapping information.

I propose that the 'has_anonymous' check should be moved out of
linux_find_memory_regions_full, and pushed into dump_mapping_p.  Then
in dump_note_entry_p there will be no has_anonymous check; it just
isn't needed.

This allows linux_find_memory_regions_full to be simplified a little,
and will allow some additional clean ups in
linux_make_mappings_callback, which is the partner function to
dump_note_entry_p (see linux_make_mappings_corefile_notes), now that
we know dump_note_entry_p is always called.  This follow on clean up
will be done in a later commit in this series.

Looking at dump_mapping_p, I do wonder if the ::has_anonymous check
could be moved later in the function.  The first few checks in
dump_mapping_p don't rely on the anonymous information, so running
them might give better results.  However, the lack of the anonymous
information is only for older kernels, so testing any changes in this
area would likely require spinning up an older kernel, and as the
years pass, we likely care about this case less.  So for now I've left
the ::has_anonymous check as the first thing in dump_mapping_p as this
keeps the existing behaviour.

There should be no user visible changes after this commit.

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 03ddcb055f1..6de2a4fda28 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -680,6 +680,11 @@ mapping_is_anonymous_p (const char *filename)
 static bool
 dump_mapping_p (filter_flags filterflags, const smaps_data &map)
 {
+  /* Older Linux kernels did not support the "Anonymous:" counter.
+     If it is missing, we can't be sure what to dump, so dump everything.  */
+  if (!map.has_anonymous)
+    return true;
+
   /* Initially, we trust in what we received from our caller.  This
      value may not be very precise (i.e., it was probably gathered
      from the permission line in the /proc/PID/smaps list, which
@@ -1580,19 +1585,8 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 
   for (const struct smaps_data &map : smaps)
     {
-      bool should_dump_p = false;
-
-      if (map.has_anonymous)
-	should_dump_p = should_dump_mapping_p (filterflags, map);
-      else
-	{
-	  /* Older Linux kernels did not support the "Anonymous:" counter.
-	     If it is missing, we can't be sure - dump all the pages.  */
-	  should_dump_p = true;
-	}
-
       /* Invoke the callback function to create the corefile segment.  */
-      if (should_dump_p)
+      if (should_dump_mapping_p (filterflags, map))
 	{
 	  func (map.start_address, map.end_address - map.start_address,
 		map.offset, map.inode, map.read, map.write, map.exec,