[RFA] linux-nat.c minor cleanup

[RFA] linux-nat.c minor cleanup

[Doug Evans]

Hi.

linux-nat.c:linux_nat_info_proc_cmd uses a long long to record a pid.
There's not much point in that, so this patch changes it to a long.
[either that or it shouldn't use strtoul to parse it :-)]

The handling of info proc cmd,cwd,exe is odd too.
They're always printed and yet argument parsing looks for them anyway,
and ignores invalid commands instead of flagging an error.
This patch adds a blurb about printing cmd,cwd,exe in the help text.
The manual already touches on this, though it doesn't precisely
say that cmd,cwd,exe are always printed.
I wouldn't mind also submitting a patch to flag unknown commands
as errors, but I'm not sure what's intended as far as the parsing
of cmd,cwd,exe.

Ok to check in?

2008-12-18  Doug Evans  <dje@google.com>

	* linux-nat.c (linux_nat_info_proc_cmd): Store pid in long instead
	of long long.
	(_initialize_linux_nat): For info proc command, document that it
	always prints the command line, cwd, and exe path name.

Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.113
diff -u -p -r1.113 linux-nat.c
--- linux-nat.c	18 Dec 2008 21:35:22 -0000	1.113
+++ linux-nat.c	19 Dec 2008 05:16:19 -0000
@@ -3597,7 +3597,7 @@ linux_nat_make_corefile_notes (bfd *obfd
 static void
 linux_nat_info_proc_cmd (char *args, int from_tty)
 {
-  long long pid = PIDGET (inferior_ptid);
+  long pid = PIDGET (inferior_ptid);
   FILE *procfile;
   char **argv = NULL;
   char buffer[MAXPATHLEN];
@@ -3661,14 +3661,14 @@ linux_nat_info_proc_cmd (char *args, int
   if (pid == 0)
     error (_("No current process: you must name one."));
 
-  sprintf (fname1, "/proc/%lld", pid);
+  sprintf (fname1, "/proc/%ld", pid);
   if (stat (fname1, &dummy) != 0)
     error (_("No /proc directory: '%s'"), fname1);
 
-  printf_filtered (_("process %lld\n"), pid);
+  printf_filtered (_("process %ld\n"), pid);
   if (cmdline_f || all)
     {
-      sprintf (fname1, "/proc/%lld/cmdline", pid);
+      sprintf (fname1, "/proc/%ld/cmdline", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
@@ -3681,7 +3681,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (cwd_f || all)
     {
-      sprintf (fname1, "/proc/%lld/cwd", pid);
+      sprintf (fname1, "/proc/%ld/cwd", pid);
       memset (fname2, 0, sizeof (fname2));
       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
 	printf_filtered ("cwd = '%s'\n", fname2);
@@ -3690,7 +3690,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (exe_f || all)
     {
-      sprintf (fname1, "/proc/%lld/exe", pid);
+      sprintf (fname1, "/proc/%ld/exe", pid);
       memset (fname2, 0, sizeof (fname2));
       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
 	printf_filtered ("exe = '%s'\n", fname2);
@@ -3699,7 +3699,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (mappings_f || all)
     {
-      sprintf (fname1, "/proc/%lld/maps", pid);
+      sprintf (fname1, "/proc/%ld/maps", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  long long addr, endaddr, size, offset, inode;
@@ -3761,7 +3761,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (status_f || all)
     {
-      sprintf (fname1, "/proc/%lld/status", pid);
+      sprintf (fname1, "/proc/%ld/status", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
@@ -3774,7 +3774,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (stat_f || all)
     {
-      sprintf (fname1, "/proc/%lld/stat", pid);
+      sprintf (fname1, "/proc/%ld/stat", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  int itmp;
@@ -4684,7 +4684,9 @@ _initialize_linux_nat (void)
   add_info ("proc", linux_nat_info_proc_cmd, _("\
 Show /proc process information about any running process.\n\
 Specify any process id, or use the program being debugged by default.\n\
-Specify any of the following keywords for detailed info:\n\
+The command line, current working directory, and full path name of the\n\
+executable are printed.\n\
+Specify any of the following keywords for additional detailed info:\n\
   mappings -- list of mapped memory regions.\n\
   stat     -- list a bunch of random process info.\n\
   status   -- list a different bunch of random process info.\n\

Re: [RFA] linux-nat.c minor cleanup

[Doug Evans]

Ping.

On Thu, Dec 18, 2008 at 9:34 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> linux-nat.c:linux_nat_info_proc_cmd uses a long long to record a pid.
> There's not much point in that, so this patch changes it to a long.
> [either that or it shouldn't use strtoul to parse it :-)]
>
> The handling of info proc cmd,cwd,exe is odd too.
> They're always printed and yet argument parsing looks for them anyway,
> and ignores invalid commands instead of flagging an error.
> This patch adds a blurb about printing cmd,cwd,exe in the help text.
> The manual already touches on this, though it doesn't precisely
> say that cmd,cwd,exe are always printed.
> I wouldn't mind also submitting a patch to flag unknown commands
> as errors, but I'm not sure what's intended as far as the parsing
> of cmd,cwd,exe.
>
> Ok to check in?
>
> 2008-12-18  Doug Evans  <dje@google.com>
>
>        * linux-nat.c (linux_nat_info_proc_cmd): Store pid in long instead
>        of long long.
>        (_initialize_linux_nat): For info proc command, document that it
>        always prints the command line, cwd, and exe path name.
>
> Index: linux-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/linux-nat.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 linux-nat.c
> --- linux-nat.c 18 Dec 2008 21:35:22 -0000      1.113
> +++ linux-nat.c 19 Dec 2008 05:16:19 -0000
> @@ -3597,7 +3597,7 @@ linux_nat_make_corefile_notes (bfd *obfd
>  static void
>  linux_nat_info_proc_cmd (char *args, int from_tty)
>  {
> -  long long pid = PIDGET (inferior_ptid);
> +  long pid = PIDGET (inferior_ptid);
>   FILE *procfile;
>   char **argv = NULL;
>   char buffer[MAXPATHLEN];
> @@ -3661,14 +3661,14 @@ linux_nat_info_proc_cmd (char *args, int
>   if (pid == 0)
>     error (_("No current process: you must name one."));
>
> -  sprintf (fname1, "/proc/%lld", pid);
> +  sprintf (fname1, "/proc/%ld", pid);
>   if (stat (fname1, &dummy) != 0)
>     error (_("No /proc directory: '%s'"), fname1);
>
> -  printf_filtered (_("process %lld\n"), pid);
> +  printf_filtered (_("process %ld\n"), pid);
>   if (cmdline_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/cmdline", pid);
> +      sprintf (fname1, "/proc/%ld/cmdline", pid);
>       if ((procfile = fopen (fname1, "r")) != NULL)
>        {
>          struct cleanup *cleanup = make_cleanup_fclose (procfile);
> @@ -3681,7 +3681,7 @@ linux_nat_info_proc_cmd (char *args, int
>     }
>   if (cwd_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/cwd", pid);
> +      sprintf (fname1, "/proc/%ld/cwd", pid);
>       memset (fname2, 0, sizeof (fname2));
>       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
>        printf_filtered ("cwd = '%s'\n", fname2);
> @@ -3690,7 +3690,7 @@ linux_nat_info_proc_cmd (char *args, int
>     }
>   if (exe_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/exe", pid);
> +      sprintf (fname1, "/proc/%ld/exe", pid);
>       memset (fname2, 0, sizeof (fname2));
>       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
>        printf_filtered ("exe = '%s'\n", fname2);
> @@ -3699,7 +3699,7 @@ linux_nat_info_proc_cmd (char *args, int
>     }
>   if (mappings_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/maps", pid);
> +      sprintf (fname1, "/proc/%ld/maps", pid);
>       if ((procfile = fopen (fname1, "r")) != NULL)
>        {
>          long long addr, endaddr, size, offset, inode;
> @@ -3761,7 +3761,7 @@ linux_nat_info_proc_cmd (char *args, int
>     }
>   if (status_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/status", pid);
> +      sprintf (fname1, "/proc/%ld/status", pid);
>       if ((procfile = fopen (fname1, "r")) != NULL)
>        {
>          struct cleanup *cleanup = make_cleanup_fclose (procfile);
> @@ -3774,7 +3774,7 @@ linux_nat_info_proc_cmd (char *args, int
>     }
>   if (stat_f || all)
>     {
> -      sprintf (fname1, "/proc/%lld/stat", pid);
> +      sprintf (fname1, "/proc/%ld/stat", pid);
>       if ((procfile = fopen (fname1, "r")) != NULL)
>        {
>          int itmp;
> @@ -4684,7 +4684,9 @@ _initialize_linux_nat (void)
>   add_info ("proc", linux_nat_info_proc_cmd, _("\
>  Show /proc process information about any running process.\n\
>  Specify any process id, or use the program being debugged by default.\n\
> -Specify any of the following keywords for detailed info:\n\
> +The command line, current working directory, and full path name of the\n\
> +executable are printed.\n\
> +Specify any of the following keywords for additional detailed info:\n\
>   mappings -- list of mapped memory regions.\n\
>   stat     -- list a bunch of random process info.\n\
>   status   -- list a different bunch of random process info.\n\
>

Re: [RFA] linux-nat.c minor cleanup

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

On Mon, Jan 5, 2009 at 3:04 PM, Doug Evans <dje@google.com> wrote:
> Ping.
>
> On Thu, Dec 18, 2008 at 9:34 PM, Doug Evans <dje@google.com> wrote:
>> Hi.
>>
>> linux-nat.c:linux_nat_info_proc_cmd uses a long long to record a pid.
>> There's not much point in that, so this patch changes it to a long.
>> [either that or it shouldn't use strtoul to parse it :-)]
>>
>> The handling of info proc cmd,cwd,exe is odd too.
>> They're always printed and yet argument parsing looks for them anyway,
>> and ignores invalid commands instead of flagging an error.
>> This patch adds a blurb about printing cmd,cwd,exe in the help text.
>> The manual already touches on this, though it doesn't precisely
>> say that cmd,cwd,exe are always printed.
>> I wouldn't mind also submitting a patch to flag unknown commands
>> as errors, but I'm not sure what's intended as far as the parsing
>> of cmd,cwd,exe.

Going over the file I found a case I missed.
atoi has undefined behaviour for invalid input so I stuck with strtoul
in linux_nat_info_proc_cmd.  Granted, it is awkward that some fns use
long and some fns use int, but ptid_get_pid returns an int and strotul
returns a long.  strtopid anyone?

[I realize this is just minor cleanup, but for completeness' sake,
there are several uses of pid_t in linux-nat.c - maybe pid_t should be
used everywhere instead of int - if one wants to be pedantic.  But
then one is left with either assuming it's an int or adding int casts
to sprintf arguments (which the code already does).  I don't have a
preference, I'll make the patch look like whatever y'all want.  It's
just annoying to see long long used here.]

This version removes the change to the doc string for "info proc".  I
could still use some input on what preferred: remove the argument
processing to watch for cmd, cwd, exe?

2008-12-18  Doug Evans  <dje@google.com>

        * linux-nat.c (linux_nat_find_memory_regions): Result of PIDGET is an
        int, not a long long.
        (linux_nat_info_proc_cmd): Store pid in long instead of long long.

[-- Attachment #2: gdb-090105-linux-nat-1.patch.txt --]
[-- Type: text/plain, Size: 3865 bytes --]

2008-12-18  Doug Evans  <dje@google.com>

	* linux-nat.c (linux_nat_find_memory_regions): Result of PIDGET is an
	int, not a long long.
	(linux_nat_info_proc_cmd): Store pid in long instead of long long.

Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.115
diff -u -p -u -p -r1.115 linux-nat.c
--- linux-nat.c	3 Jan 2009 05:57:52 -0000	1.115
+++ linux-nat.c	5 Jan 2009 23:41:22 -0000
@@ -3346,7 +3346,7 @@ linux_nat_find_memory_regions (int (*fun
 					    unsigned long,
 					    int, int, int, void *), void *obfd)
 {
-  long long pid = PIDGET (inferior_ptid);
+  int pid = PIDGET (inferior_ptid);
   char mapsfilename[MAXPATHLEN];
   FILE *mapsfile;
   long long addr, endaddr, size, offset, inode;
@@ -3356,7 +3356,7 @@ linux_nat_find_memory_regions (int (*fun
   struct cleanup *cleanup;
 
   /* Compose the filename for the /proc memory map, and open it.  */
-  sprintf (mapsfilename, "/proc/%lld/maps", pid);
+  sprintf (mapsfilename, "/proc/%d/maps", pid);
   if ((mapsfile = fopen (mapsfilename, "r")) == NULL)
     error (_("Could not open %s."), mapsfilename);
   cleanup = make_cleanup_fclose (mapsfile);
@@ -3609,7 +3609,9 @@ linux_nat_make_corefile_notes (bfd *obfd
 static void
 linux_nat_info_proc_cmd (char *args, int from_tty)
 {
-  long long pid = PIDGET (inferior_ptid);
+  /* A long is used for pid instead of an int to avoid a loss of precision
+     compiler warning from the output of strtoul.  */
+  long pid = PIDGET (inferior_ptid);
   FILE *procfile;
   char **argv = NULL;
   char buffer[MAXPATHLEN];
@@ -3673,14 +3675,14 @@ linux_nat_info_proc_cmd (char *args, int
   if (pid == 0)
     error (_("No current process: you must name one."));
 
-  sprintf (fname1, "/proc/%lld", pid);
+  sprintf (fname1, "/proc/%ld", pid);
   if (stat (fname1, &dummy) != 0)
     error (_("No /proc directory: '%s'"), fname1);
 
-  printf_filtered (_("process %lld\n"), pid);
+  printf_filtered (_("process %ld\n"), pid);
   if (cmdline_f || all)
     {
-      sprintf (fname1, "/proc/%lld/cmdline", pid);
+      sprintf (fname1, "/proc/%ld/cmdline", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
@@ -3693,7 +3695,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (cwd_f || all)
     {
-      sprintf (fname1, "/proc/%lld/cwd", pid);
+      sprintf (fname1, "/proc/%ld/cwd", pid);
       memset (fname2, 0, sizeof (fname2));
       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
 	printf_filtered ("cwd = '%s'\n", fname2);
@@ -3702,7 +3704,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (exe_f || all)
     {
-      sprintf (fname1, "/proc/%lld/exe", pid);
+      sprintf (fname1, "/proc/%ld/exe", pid);
       memset (fname2, 0, sizeof (fname2));
       if (readlink (fname1, fname2, sizeof (fname2)) > 0)
 	printf_filtered ("exe = '%s'\n", fname2);
@@ -3711,7 +3713,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (mappings_f || all)
     {
-      sprintf (fname1, "/proc/%lld/maps", pid);
+      sprintf (fname1, "/proc/%ld/maps", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  long long addr, endaddr, size, offset, inode;
@@ -3773,7 +3775,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (status_f || all)
     {
-      sprintf (fname1, "/proc/%lld/status", pid);
+      sprintf (fname1, "/proc/%ld/status", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
@@ -3786,7 +3788,7 @@ linux_nat_info_proc_cmd (char *args, int
     }
   if (stat_f || all)
     {
-      sprintf (fname1, "/proc/%lld/stat", pid);
+      sprintf (fname1, "/proc/%ld/stat", pid);
       if ((procfile = fopen (fname1, "r")) != NULL)
 	{
 	  int itmp;

Re: [RFA] linux-nat.c minor cleanup

[Doug Evans]

Ping.

On Mon, Jan 5, 2009 at 5:00 PM, Doug Evans <dje@google.com> wrote:
> On Mon, Jan 5, 2009 at 3:04 PM, Doug Evans <dje@google.com> wrote:
>> Ping.
>>
>> On Thu, Dec 18, 2008 at 9:34 PM, Doug Evans <dje@google.com> wrote:
>>> Hi.
>>>
>>> linux-nat.c:linux_nat_info_proc_cmd uses a long long to record a pid.
>>> There's not much point in that, so this patch changes it to a long.
>>> [either that or it shouldn't use strtoul to parse it :-)]
>>>
>>> The handling of info proc cmd,cwd,exe is odd too.
>>> They're always printed and yet argument parsing looks for them anyway,
>>> and ignores invalid commands instead of flagging an error.
>>> This patch adds a blurb about printing cmd,cwd,exe in the help text.
>>> The manual already touches on this, though it doesn't precisely
>>> say that cmd,cwd,exe are always printed.
>>> I wouldn't mind also submitting a patch to flag unknown commands
>>> as errors, but I'm not sure what's intended as far as the parsing
>>> of cmd,cwd,exe.
>
> Going over the file I found a case I missed.
> atoi has undefined behaviour for invalid input so I stuck with strtoul
> in linux_nat_info_proc_cmd.  Granted, it is awkward that some fns use
> long and some fns use int, but ptid_get_pid returns an int and strotul
> returns a long.  strtopid anyone?
>
> [I realize this is just minor cleanup, but for completeness' sake,
> there are several uses of pid_t in linux-nat.c - maybe pid_t should be
> used everywhere instead of int - if one wants to be pedantic.  But
> then one is left with either assuming it's an int or adding int casts
> to sprintf arguments (which the code already does).  I don't have a
> preference, I'll make the patch look like whatever y'all want.  It's
> just annoying to see long long used here.]
>
> This version removes the change to the doc string for "info proc".  I
> could still use some input on what preferred: remove the argument
> processing to watch for cmd, cwd, exe?
>
> 2008-12-18  Doug Evans  <dje@google.com>
>
>        * linux-nat.c (linux_nat_find_memory_regions): Result of PIDGET is an
>        int, not a long long.
>        (linux_nat_info_proc_cmd): Store pid in long instead of long long.
>

Re: [RFA] linux-nat.c minor cleanup

[Pedro Alves]

On Friday 13 March 2009 17:10:06, Doug Evans wrote:

> > 2008-12-18  Doug Evans  <dje@google.com>
> >
> >        * linux-nat.c (linux_nat_find_memory_regions): Result of PIDGET is an
> >        int, not a long long.
> >        (linux_nat_info_proc_cmd): Store pid in long instead of long long.
> >
> 

Ok.

-- 
Pedro Alves