[PATCH 1/2] gdb/linux-tdep: make read_mapping return a structure

[PATCH 1/2] gdb/linux-tdep: make read_mapping return a structure

[Simon Marchi]

Change read_mapping to return a structure instead of taking many output
parameters.  Change the string + length output parameters (permissions
and device) to be gdb::string_view, since that's what string_view is
for (a non-NULL terminated view on a string).  No changes in behavior
expected.

Change-Id: I86e627d84d3dda8c9b835592b0f4de8d90d12112
---
 gdb/linux-tdep.c | 107 ++++++++++++++++++++++++-----------------------
 1 file changed, 55 insertions(+), 52 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 060f60e753a2..4b94a442e566 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -441,42 +441,53 @@ linux_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid)
   return normal_pid_to_str (ptid);
 }
 
+struct mapping
+{
+  ULONGEST addr;
+  ULONGEST endaddr;
+  gdb::string_view permissions;
+  ULONGEST offset;
+  gdb::string_view device;
+  ULONGEST inode;
+
+  /* This field is guaranteed to be NULL-terminated, hence it is not a
+     gdb::string_view.  */
+  const char *filename;
+};
+
 /* Service function for corefiles and info proc.  */
 
-static void
-read_mapping (const char *line,
-	      ULONGEST *addr, ULONGEST *endaddr,
-	      const char **permissions, size_t *permissions_len,
-	      ULONGEST *offset,
-	      const char **device, size_t *device_len,
-	      ULONGEST *inode,
-	      const char **filename)
+static mapping
+read_mapping (const char *line)
 {
+  struct mapping mapping;
   const char *p = line;
 
-  *addr = strtoulst (p, &p, 16);
+  mapping.addr = strtoulst (p, &p, 16);
   if (*p == '-')
     p++;
-  *endaddr = strtoulst (p, &p, 16);
+  mapping.endaddr = strtoulst (p, &p, 16);
 
   p = skip_spaces (p);
-  *permissions = p;
+  const char *permissions_start = p;
   while (*p && !isspace (*p))
     p++;
-  *permissions_len = p - *permissions;
+  mapping.permissions = {permissions_start, (size_t) (p - permissions_start)};
 
-  *offset = strtoulst (p, &p, 16);
+  mapping.offset = strtoulst (p, &p, 16);
 
   p = skip_spaces (p);
-  *device = p;
+  const char *device_start = p;
   while (*p && !isspace (*p))
     p++;
-  *device_len = p - *device;
+  mapping.device = {device_start, (size_t) (p - device_start)};
 
-  *inode = strtoulst (p, &p, 10);
+  mapping.inode = strtoulst (p, &p, 10);
 
   p = skip_spaces (p);
-  *filename = p;
+  mapping.filename = p;
+
+  return mapping;
 }
 
 /* Helper function to decode the "VmFlags" field in /proc/PID/smaps.
@@ -895,34 +906,27 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 	       line;
 	       line = strtok_r (NULL, "\n", &saveptr))
 	    {
-	      ULONGEST addr, endaddr, offset, inode;
-	      const char *permissions, *device, *mapping_filename;
-	      size_t permissions_len, device_len;
-
-	      read_mapping (line, &addr, &endaddr,
-			    &permissions, &permissions_len,
-			    &offset, &device, &device_len,
-			    &inode, &mapping_filename);
+	      struct mapping m = read_mapping (line);
 
 	      if (gdbarch_addr_bit (gdbarch) == 32)
 		{
 		  printf_filtered ("\t%10s %10s %7.5s %10s %10s %s\n",
-				   paddress (gdbarch, addr),
-				   paddress (gdbarch, endaddr),
-				   permissions,
-				   hex_string (endaddr - addr),
-				   hex_string (offset),
-				   *mapping_filename ? mapping_filename : "");
+				   paddress (gdbarch, m.addr),
+				   paddress (gdbarch, m.endaddr),
+				   m.permissions.data (),
+				   hex_string (m.endaddr - m.addr),
+				   hex_string (m.offset),
+				   m.filename);
 		}
 	      else
 		{
 		  printf_filtered ("  %18s %18s %7.5s %10s %10s %s\n",
-				   paddress (gdbarch, addr),
-				   paddress (gdbarch, endaddr),
-				   permissions,
-				   hex_string (endaddr - addr),
-				   hex_string (offset),
-				   *mapping_filename ? mapping_filename : "");
+				   paddress (gdbarch, m.addr),
+				   paddress (gdbarch, m.endaddr),
+				   m.permissions.data (),
+				   hex_string (m.endaddr - m.addr),
+				   hex_string (m.offset),
+				   m.filename);
 		}
 	    }
 	}
@@ -1322,19 +1326,15 @@ parse_smaps_data (const char *data,
 
   while (line != NULL)
     {
-      ULONGEST addr, endaddr, offset, inode;
-      const char *permissions, *device, *filename;
       struct smaps_vmflags v;
-      size_t permissions_len, device_len;
       int read, write, exec, priv;
       int has_anonymous = 0;
       int mapping_anon_p;
       int mapping_file_p;
 
       memset (&v, 0, sizeof (v));
-      read_mapping (line, &addr, &endaddr, &permissions, &permissions_len,
-		    &offset, &device, &device_len, &inode, &filename);
-      mapping_anon_p = mapping_is_anonymous_p (filename);
+      struct mapping m = read_mapping (line);
+      mapping_anon_p = mapping_is_anonymous_p (m.filename);
       /* If the mapping is not anonymous, then we can consider it
 	 to be file-backed.  These two states (anonymous or
 	 file-backed) seem to be exclusive, but they can actually
@@ -1347,9 +1347,12 @@ parse_smaps_data (const char *data,
       mapping_file_p = !mapping_anon_p;
 
       /* Decode permissions.  */
-      read = (memchr (permissions, 'r', permissions_len) != 0);
-      write = (memchr (permissions, 'w', permissions_len) != 0);
-      exec = (memchr (permissions, 'x', permissions_len) != 0);
+      auto has_perm = [&m] (char c)
+	{ return m.permissions.find (c) != gdb::string_view::npos; };
+      read = has_perm ('r');
+      write = has_perm ('w');
+      exec = has_perm ('x');
+
       /* 'private' here actually means VM_MAYSHARE, and not
 	 VM_SHARED.  In order to know if a mapping is really
 	 private or not, we must check the flag "sh" in the
@@ -1359,7 +1362,7 @@ parse_smaps_data (const char *data,
 	 not have the VmFlags there.  In this case, there is
 	 really no way to know if we are dealing with VM_SHARED,
 	 so we just assume that VM_MAYSHARE is enough.  */
-      priv = memchr (permissions, 'p', permissions_len) != 0;
+      priv = has_perm ('p');
 
       /* Try to detect if region should be dumped by parsing smaps
 	 counters.  */
@@ -1421,9 +1424,9 @@ parse_smaps_data (const char *data,
       /* Save the smaps entry to the vector.  */
 	struct smaps_data map;
 
-	map.start_address = addr;
-	map.end_address = endaddr;
-	map.filename = filename;
+	map.start_address = m.addr;
+	map.end_address = m.endaddr;
+	map.filename = m.filename;
 	map.vmflags = v;
 	map.read = read? true : false;
 	map.write = write? true : false;
@@ -1432,8 +1435,8 @@ parse_smaps_data (const char *data,
 	map.has_anonymous = has_anonymous;
 	map.mapping_anon_p = mapping_anon_p? true : false;
 	map.mapping_file_p = mapping_file_p? true : false;
-	map.offset = offset;
-	map.inode = inode;
+	map.offset = m.offset;
+	map.inode = m.inode;
 
 	smaps.emplace_back (map);
     }

base-commit: ac03c8d8fd6cf7f9080068589683cb06531879c2
-- 
2.35.1

[PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Simon Marchi]

Commit 29ef4c0699e1 ("gdb/linux-tdep.c: Add Perms to the 'info proc
mappings' output") has broken test gdb.base/info-proc.exp on Linux,
because it changes the output of "info proc mappings" in a way that the
test does not expect (my bad for not testing before pushing).

I looked at how FreeBSD handles this, since I remembered it did show
permission flags.  It looks like this:

          Start Addr           End Addr       Size     Offset   Flags   File
            0x200000           0x243000    0x43000        0x0  r-- CN-- /usr/local/bin/tmux

(I think that `Flags` and the flags not being aligned is not
intentional)

The test passes on FreeBSD, because the test looks for four hex numbers
in a row and ignores the rest:

    ".*Mapped address spaces:.*${hex}${ws}${hex}${ws}${hex}${ws}${hex}.*"

I suggest fixing it on Linux by moving the flags column to the same
place as in the FreeBSD output.  It makes things a bit more consistent
between OSes, and we don't have to touch the test.

At the same time, make use of the actual length of the permission's
string to specify the number of characters to print.

Before this patch, the output looks like:

          Start Addr           End Addr   Perms       Size     Offset objfile
      0x55dd4b544000     0x55dd4b546000   r--p      0x2000        0x0 /usr/bin/sleep

and after, it looks like:

          Start Addr           End Addr       Size     Offset  Perms  objfile
      0x5622ae662000     0x5622ae664000     0x2000        0x0  r--p   /usr/bin/sleep

Change-Id: If0fc167b010b25f97a3c54e2f491df4973ccde8f
---
 gdb/linux-tdep.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 4b94a442e566..dc153e565374 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -890,15 +890,15 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 	  printf_filtered (_("Mapped address spaces:\n\n"));
 	  if (gdbarch_addr_bit (gdbarch) == 32)
 	    {
-	      printf_filtered ("\t%10s %10s %7s %10s %10s %s\n",
-			   "Start Addr", "  End Addr", "Perms",
-			   "      Size", "    Offset", "objfile");
+	      printf_filtered ("\t%10s %10s %10s %10s  %s %s\n",
+			       "Start Addr", "  End Addr", "      Size",
+			       "    Offset", "Perms  ", "objfile");
 	    }
 	  else
 	    {
-	      printf_filtered ("  %18s %18s %7s %10s %10s %s\n",
-			   "Start Addr", "  End Addr", "Perms",
-			   "      Size", "    Offset", "objfile");
+	      printf_filtered ("  %18s %18s %10s %10s  %s %s\n",
+			       "Start Addr", "  End Addr", "      Size",
+			       "    Offset", "Perms ", "objfile");
 	    }
 
 	  char *saveptr;
@@ -910,22 +910,24 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 
 	      if (gdbarch_addr_bit (gdbarch) == 32)
 		{
-		  printf_filtered ("\t%10s %10s %7.5s %10s %10s %s\n",
+		  printf_filtered ("\t%10s %10s %10s %10s  %-5.*s  %s\n",
 				   paddress (gdbarch, m.addr),
 				   paddress (gdbarch, m.endaddr),
-				   m.permissions.data (),
 				   hex_string (m.endaddr - m.addr),
 				   hex_string (m.offset),
+				   (int) m.permissions.size (),
+				   m.permissions.data (),
 				   m.filename);
 		}
 	      else
 		{
-		  printf_filtered ("  %18s %18s %7.5s %10s %10s %s\n",
+		  printf_filtered ("  %18s %18s %10s %10s  %-5.*s  %s\n",
 				   paddress (gdbarch, m.addr),
 				   paddress (gdbarch, m.endaddr),
-				   m.permissions.data (),
 				   hex_string (m.endaddr - m.addr),
 				   hex_string (m.offset),
+				   (int) m.permissions.size (),
+				   m.permissions.data (),
 				   m.filename);
 		}
 	    }
-- 
2.35.1

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[John Baldwin]

On 2/23/22 1:26 PM, Simon Marchi via Gdb-patches wrote:
> Commit 29ef4c0699e1 ("gdb/linux-tdep.c: Add Perms to the 'info proc
> mappings' output") has broken test gdb.base/info-proc.exp on Linux,
> because it changes the output of "info proc mappings" in a way that the
> test does not expect (my bad for not testing before pushing).
> 
> I looked at how FreeBSD handles this, since I remembered it did show
> permission flags.  It looks like this:
> 
>            Start Addr           End Addr       Size     Offset   Flags   File
>              0x200000           0x243000    0x43000        0x0  r-- CN-- /usr/local/bin/tmux
> 
> (I think that `Flags` and the flags not being aligned is not
> intentional)
> 
> The test passes on FreeBSD, because the test looks for four hex numbers
> in a row and ignores the rest:
> 
>      ".*Mapped address spaces:.*${hex}${ws}${hex}${ws}${hex}${ws}${hex}.*"
> 
> I suggest fixing it on Linux by moving the flags column to the same
> place as in the FreeBSD output.  It makes things a bit more consistent
> between OSes, and we don't have to touch the test.

FWIW, the format I chose to use on FreeBSD was to match an existing tool
in FreeBSD (procstat(1)).  I wasn't necessarily trying to come up with a
cross-platform format.  The weird alignment of "Flags" is on purpose as it
spans both the permissions field and a separate "flags" field (the 'C'
stands for a Copy-On-Write mapping for example).

(https://www.freebsd.org/cgi/man.cgi?procstat describes possible flags)

I think your change is fine though.

-- 
John Baldwin

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Simon Marchi]

On 2022-02-23 16:41, John Baldwin wrote:
> On 2/23/22 1:26 PM, Simon Marchi via Gdb-patches wrote:
>> Commit 29ef4c0699e1 ("gdb/linux-tdep.c: Add Perms to the 'info proc
>> mappings' output") has broken test gdb.base/info-proc.exp on Linux,
>> because it changes the output of "info proc mappings" in a way that the
>> test does not expect (my bad for not testing before pushing).
>>
>> I looked at how FreeBSD handles this, since I remembered it did show
>> permission flags.  It looks like this:
>>
>>            Start Addr           End Addr       Size     Offset   Flags   File
>>              0x200000           0x243000    0x43000        0x0  r-- CN-- /usr/local/bin/tmux
>>
>> (I think that `Flags` and the flags not being aligned is not
>> intentional)
>>
>> The test passes on FreeBSD, because the test looks for four hex numbers
>> in a row and ignores the rest:
>>
>>      ".*Mapped address spaces:.*${hex}${ws}${hex}${ws}${hex}${ws}${hex}.*"
>>
>> I suggest fixing it on Linux by moving the flags column to the same
>> place as in the FreeBSD output.  It makes things a bit more consistent
>> between OSes, and we don't have to touch the test.
> 
> FWIW, the format I chose to use on FreeBSD was to match an existing tool
> in FreeBSD (procstat(1)).  I wasn't necessarily trying to come up with a
> cross-platform format.

That's fine, and I don't think we can really have a cross-platform
format, each OS has different concepts.

> The weird alignment of "Flags" is on purpose as it
> spans both the permissions field and a separate "flags" field (the 'C'
> stands for a Copy-On-Write mapping for example).

Ah ok, so it's centered on the column.

> (https://www.freebsd.org/cgi/man.cgi?procstat describes possible flags)
> 
> I think your change is fine though.

Thanks.  Adding Dominik in CC, I realize he is probably not subscribed
to gdb-patches.  Does that look good to you?  Here's the link to the
patch if needed:

  https://sourceware.org/pipermail/gdb-patches/2022-February/186083.html

Simon

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Dominik Czarnota]

Hey,

On Thu, 24 Feb 2022 at 13:18, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2022-02-23 16:41, John Baldwin wrote:
> > On 2/23/22 1:26 PM, Simon Marchi via Gdb-patches wrote:
> >> Commit 29ef4c0699e1 ("gdb/linux-tdep.c: Add Perms to the 'info proc
> >> mappings' output") has broken test gdb.base/info-proc.exp on Linux,
> >> because it changes the output of "info proc mappings" in a way that the
> >> test does not expect (my bad for not testing before pushing).
> >>
> >> I looked at how FreeBSD handles this, since I remembered it did show
> >> permission flags.  It looks like this:
> >>
> >>            Start Addr           End Addr       Size     Offset   Flags
>  File
> >>              0x200000           0x243000    0x43000        0x0  r--
> CN-- /usr/local/bin/tmux
> >>
> >> (I think that `Flags` and the flags not being aligned is not
> >> intentional)
> >>
> >> The test passes on FreeBSD, because the test looks for four hex numbers
> >> in a row and ignores the rest:
> >>
> >>      ".*Mapped address
> spaces:.*${hex}${ws}${hex}${ws}${hex}${ws}${hex}.*"
> >>
> >> I suggest fixing it on Linux by moving the flags column to the same
> >> place as in the FreeBSD output.  It makes things a bit more consistent
> >> between OSes, and we don't have to touch the test.
> >
> > FWIW, the format I chose to use on FreeBSD was to match an existing tool
> > in FreeBSD (procstat(1)).  I wasn't necessarily trying to come up with a
> > cross-platform format.
>
> That's fine, and I don't think we can really have a cross-platform
> format, each OS has different concepts.
>
> > The weird alignment of "Flags" is on purpose as it
> > spans both the permissions field and a separate "flags" field (the 'C'
> > stands for a Copy-On-Write mapping for example).
>
> Ah ok, so it's centered on the column.
>
> > (https://www.freebsd.org/cgi/man.cgi?procstat describes possible flags)
> >
> > I think your change is fine though.
>
> Thanks.  Adding Dominik in CC, I realize he is probably not subscribed
> to gdb-patches.  Does that look good to you?  Here's the link to the
> patch if needed:
>
>   https://sourceware.org/pipermail/gdb-patches/2022-February/186083.html

Yeah, it looks good to me. Thanks a lot for taking care of this.


>
>
> Simon
>

Best regards,
Dominik

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Simon Marchi]

> Yeah, it looks good to me. Thanks a lot for taking care of this.

Thanks, pushed.

Simon

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> -	      printf_filtered ("  %18s %18s %7s %10s %10s %s\n",
Simon> -			   "Start Addr", "  End Addr", "Perms",
Simon> -			   "      Size", "    Offset", "objfile");
Simon> +	      printf_filtered ("  %18s %18s %10s %10s  %s %s\n",
Simon> +			       "Start Addr", "  End Addr", "      Size",
Simon> +			       "    Offset", "Perms ", "objfile");

I'm not requesting any sort of change, but it's often nice to use ui_out
for this kind of thing, since it handles the table formatting, and also
stuff like styling for addresses.

Tom

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Dominik Czarnota]

Hey,

I can see this change in latest release 12.1 but it is not noted in
the changelog e.g. on  https://www.sourceware.org/gdb/.
Can we maybe still add it on the release changelog? Could be sth like:
- The 'info proc mappings' command will now display a column with
memory protections (similar as present in the proc filesystem
/proc/$pid/maps file on Linux)

Thanks,
Dominik

On Thu, 24 Feb 2022 at 13:29, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> > Yeah, it looks good to me. Thanks a lot for taking care of this.
>
> Thanks, pushed.
>
> Simon

Re: [PATCH 2/2] gdb/linux-tdep: move "Perms" column right

[Simon Marchi]

On 2022-05-04 18:04, Dominik Czarnota wrote:
> Hey,
> 
> I can see this change in latest release 12.1 but it is not noted in
> the changelog e.g. on  https://www.sourceware.org/gdb/.
> Can we maybe still add it on the release changelog? Could be sth like:
> - The 'info proc mappings' command will now display a column with
> memory protections (similar as present in the proc filesystem
> /proc/$pid/maps file on Linux)
> 
> Thanks,
> Dominik

Hi Dominik,

Feel free to propose a patch for the NEWS file, that could be applied
on gdb-12-branch.  However, I'm not sure it would make it to the web
page, as it was not preset in the 12.1 release (and it would be weird
for the website to not be in sync with the release).

Simon