--pid and --core

--pid and --core

[Pedro Alves]

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

Hi all,

I was looking through --pid and attach mingw PRs in the GNATS,
and noticed an annoying issue.

If one specifies an explicit PID to attach to, failing to
attach, gdb will try to open a core file of the same name.

The attach fail -> open core is there to support
invoking gdb as:

      gdb program pid

or:

      gdb program core

Since, if the user specifies gdb program 3333, gdb can't
know if 3333 is a pid or a core file named "3333".

So for, so good.  Now,

If the user specifies,

      gdb program --pid=3333,

gdb has no business trying to look for a core file named 3333.

The patch avoids these confusing messages [1] when the user specifies
an invalid or non-existing pid:

Before:

     >./gdb --pid=3333
     Attaching to process 3333
     ptrace: No such process.
     /home/pedro/gdb/build/3333: No such file or directory.  <==== [1]
     (gdb)

After:

     >./gdb --pid=3333
     Attaching to process 3333
     ptrace: No such process.
     (gdb)

The patch also catches invalid simultaneous --pid,--core uses,

And these weirdnesses:

Before:

   >gdb/gdb gdb/gdb --pid=3333 4444 5555
   Excess command line arguments ignored. (5555)
   Attaching to program: /home/pedro/gdb/build/gdb/gdb, process 4444
   ptrace: No such process.
   /home/pedro/gdb/build/4444: No such file or directory.

After:

   >gdb/gdb gdb/gdb --pid=3333 4444 5555
   Excess command line arguments ignored. (4444 ...)
   Attaching to program: /home/pedro/gdb/build/gdb/gdb, process 3333
   ptrace: No such process.

Tested on i686-pc-linux-gnu, no regressions.

OK ?



[-- Attachment #2: pid_core_split.diff --]
[-- Type: text/x-diff, Size: 4149 bytes --]

2007-12-28  Pedro Alves  <pedro_alves@portugalmail.pt>

	* main.c (captured_main): Error out if --core and --pid options
	were issued simultaneously.  If an explicit pid option was passed,
	don't fallback to core file.  Detect extra arguments better in the
	presence of explicit pid or core arguments.

---
 gdb/main.c |   76 ++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 28 deletions(-)

Index: src/gdb/main.c
===================================================================
--- src.orig/gdb/main.c	2008-01-02 17:58:39.000000000 +0000
+++ src/gdb/main.c	2008-01-03 16:43:03.000000000 +0000
@@ -127,7 +127,9 @@ captured_main (void *data)
   /* Pointers to various arguments from command line.  */
   char *symarg = NULL;
   char *execarg = NULL;
+  char *pidarg = NULL;
   char *corearg = NULL;
+  char *pid_or_core_arg = NULL;
   char *cdarg = NULL;
   char *ttyarg = NULL;
 
@@ -435,8 +437,7 @@ captured_main (void *data)
 	    corearg = optarg;
 	    break;
 	  case 'p':
-	    /* "corearg" is shared by "--core" and "--pid" */
-	    corearg = optarg;
+	    pidarg = optarg;
 	    break;
 	  case 'x':
 	    cmdarg[ncmd].type = CMDARG_FILE;
@@ -575,23 +576,32 @@ extern int gdbtk_test (char *);
 	/* OK, that's all the options.  The other arguments are filenames.  */
 	count = 0;
 	for (; optind < argc; optind++)
-	  switch (++count)
-	    {
-	    case 1:
-	      symarg = argv[optind];
-	      execarg = argv[optind];
-	      break;
-	    case 2:
-	      /* The documentation says this can be a "ProcID" as well. 
-	         We will try it as both a corefile and a pid.  */
-	      corearg = argv[optind];
-	      break;
-	    case 3:
-	      fprintf_unfiltered (gdb_stderr,
-				  _("Excess command line arguments ignored. (%s%s)\n"),
-				  argv[optind], (optind == argc - 1) ? "" : " ...");
-	      break;
-	    }
+	  {
+	    ++count;
+	    if (count == 1)
+	      {
+		symarg = argv[optind];
+		execarg = argv[optind];
+	      }
+	    else if (count > 2
+		     /* If we have a --pid or a --core argument,
+			this argument don't make sense.  */
+		     || pidarg != NULL
+		     || corearg != NULL)
+	      {
+		fprintf_unfiltered (gdb_stderr, _("\
+Excess command line arguments ignored. (%s%s)\n"),
+				    argv[optind],
+				    (optind == argc - 1) ? "" : " ...");
+		optind = argc;
+		break;
+	      }
+	    else
+	      /* The documentation says this can be a "ProcID" as
+		 well.  We will try it later as both a corefile and a
+		 pid.  */
+	      pid_or_core_arg = argv[optind];
+	  }
       }
     if (batch)
       quiet = 1;
@@ -733,21 +743,31 @@ extern int gdbtk_test (char *);
 	catch_command_errors (symbol_file_add_main, symarg, 0, RETURN_MASK_ALL);
     }
 
+  if (corearg && pidarg)
+    error (_("\
+Can't attach to process and specify a core file at the same time."));
+
   if (corearg != NULL)
-    {
-      /* corearg may be either a corefile or a pid.
-	 If its first character is a digit, try attach first
-	 and then corefile.  Otherwise try corefile first. */
+    catch_command_errors (core_file_command, corearg,
+			  !batch, RETURN_MASK_ALL);
+  else if (pidarg != NULL)
+    catch_command_errors (attach_command, pidarg,
+			  !batch, RETURN_MASK_ALL);
+  else if (pid_or_core_arg)
+    {
+      /* The user specified 'gdb program pid' or gdb program core'.
+	 If pid_or_core_arg's first character is a digit, try attach
+	 first and then corefile.  Otherwise try just corefile.  */
 
-      if (isdigit (corearg[0]))
+      if (isdigit (pid_or_core_arg[0]))
 	{
-	  if (catch_command_errors (attach_command, corearg, 
+	  if (catch_command_errors (attach_command, pid_or_core_arg,
 				    !batch, RETURN_MASK_ALL) == 0)
-	    catch_command_errors (core_file_command, corearg, 
+	    catch_command_errors (core_file_command, pid_or_core_arg,
 				  !batch, RETURN_MASK_ALL);
 	}
-      else /* Can't be a pid, better be a corefile. */
-	catch_command_errors (core_file_command, corearg, 
+      else /* Can't be a pid, better be a corefile.  */
+	catch_command_errors (core_file_command, pid_or_core_arg,
 			      !batch, RETURN_MASK_ALL);
     }
 

Re: --pid and --core

[Joel Brobecker]

Hi Pedro,

> 2007-12-28  Pedro Alves  <pedro_alves@portugalmail.pt>
> 
> 	* main.c (captured_main): Error out if --core and --pid options
> 	were issued simultaneously.  If an explicit pid option was passed,
> 	don't fallback to core file.  Detect extra arguments better in the
> 	presence of explicit pid or core arguments.

Thanks for the patch. A couple of of comments:

> +	  {
> +	    ++count;
> +	    if (count == 1)
> +	      {
> +		symarg = argv[optind];
> +		execarg = argv[optind];
> +	      }
> +	    else if (count > 2
> +		     /* If we have a --pid or a --core argument,
> +			this argument don't make sense.  */
                                      ^^^^
                                      doesn't

> +		     || pidarg != NULL
> +		     || corearg != NULL)
> +	      {
> +		fprintf_unfiltered (gdb_stderr, _("\
> +Excess command line arguments ignored. (%s%s)\n"),
> +				    argv[optind],
> +				    (optind == argc - 1) ? "" : " ...");
> +		optind = argc;
> +		break;
> +	      }
> +	    else
> +	      /* The documentation says this can be a "ProcID" as
> +		 well.  We will try it later as both a corefile and a
> +		 pid.  */
> +	      pid_or_core_arg = argv[optind];
> +	  }

It took me a long while to understand this loop. I realize this was
already implemented that way before, but I think something like the code
below is simpler. What do you think?

  /* The first argument, if specified, is the name of the executable.  */ 
  if (argc - optind > 0)
    {
      symarg = argv[optind];
      execarg = argv[optind];
      optind++;
    }

  /* If the user hasn't already specified a PID or the name of
     a core file, then a second optional argument is allowed.
     If present, this argument should be interpreted as either
     a PID or a core file, whichever works.  */
  if (pidarg == NULL && corearg == NULL && argc - optind > 0)
    {
      pid_or_core_arg = argv[optind];
      optind++;
    }

  /* Any argument left on the command line is unexpected and will be
     ignored.  Inform the user.  */
  for (; optind < argc; optind++)
    fprintf_unfiltered (...);

Thanks,
-- 
Joel

Re: --pid and --core

[Pedro Alves]

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

Joel Brobecker wrote:
> Thanks for the patch. A couple of of comments:
> 

Thanks for taking a look!

> It took me a long while to understand this loop. I realize this was
> already implemented that way before, but I think something like the code
> below is simpler. What do you think?
> 

I completely agree.  It looks much cleaner.  Thanks.

>   if (argc - optind > 0)

I just changed these to use the (optind < argc) idiom, which is
already used in the file.

>   /* Any argument left on the command line is unexpected and will be
>      ignored.  Inform the user.  */
>   for (; optind < argc; optind++)
>     fprintf_unfiltered (...);
> 

I'm keeping the output format (ARG) or (ARG ...) if (optind < argc).

There's a wrinkle.  The 'int count' variable that was originally
used in the loop, was also used to do a last resort stack alignment in
captured_main's entry.

#if defined (ALIGN_STACK_ON_STARTUP)
      i = (int) &count & 0x03;
      if (i != 0)
        alloca (4 - i);
#endif

ALIGN_STACK_ON_STARTUP isn't defined in any target/host
anymore, and if it was, count is in the middle of the stack frame
for long and nobody complained.  We can safely
remove it.  It's mentioned in the gdbint manual, so that'll
go too.

Now with correct email in the ChangeLog entry ...

Retested on i686-pc-linux-gnu.

OK ?

Eli, gdbint patch OK ?

-- 
Pedro Alves




[-- Attachment #2: pid_core_split.diff --]
[-- Type: text/x-diff, Size: 6022 bytes --]

2008-01-03  Pedro Alves  <pedro@codesourcery.com>

	gdb/

	* main.c (captured_main): Remove 'count' varible and the
	ALIGN_STACK_ON_ENTRY block that used it.  Error out if --core and
	--pid options were issued simultaneously.  If an explicit pid
	option was passed, don't fallback to core file.  Detect extra
	arguments better in the presence of explicit pid or core
	arguments.

2008-01-03  Pedro Alves  <pedro@codesourcery.com>

	gdb/doc/

	* gdbint.texinfo (Host Conditionals): Remove mention of
	ALIGN_STACK_ON_ENTRY.

---
 gdb/doc/gdbint.texinfo |    7 ---
 gdb/main.c             |   88 +++++++++++++++++++++++++++----------------------
 2 files changed, 50 insertions(+), 45 deletions(-)

Index: src/gdb/main.c
===================================================================
--- src.orig/gdb/main.c	2008-01-02 17:58:39.000000000 +0000
+++ src/gdb/main.c	2008-01-04 11:36:05.000000000 +0000
@@ -119,7 +119,6 @@ captured_main (void *data)
   struct captured_main_args *context = data;
   int argc = context->argc;
   char **argv = context->argv;
-  int count;
   static int quiet = 0;
   static int batch = 0;
   static int set_args = 0;
@@ -127,7 +126,9 @@ captured_main (void *data)
   /* Pointers to various arguments from command line.  */
   char *symarg = NULL;
   char *execarg = NULL;
+  char *pidarg = NULL;
   char *corearg = NULL;
+  char *pid_or_core_arg = NULL;
   char *cdarg = NULL;
   char *ttyarg = NULL;
 
@@ -175,12 +176,6 @@ captured_main (void *data)
   lim_at_start = (char *) sbrk (0);
 #endif
 
-#if defined (ALIGN_STACK_ON_STARTUP)
-  i = (int) &count & 0x3;
-  if (i != 0)
-    alloca (4 - i);
-#endif
-
   cmdsize = 1;
   cmdarg = (struct cmdarg *) xmalloc (cmdsize * sizeof (*cmdarg));
   ncmd = 0;
@@ -435,8 +430,7 @@ captured_main (void *data)
 	    corearg = optarg;
 	    break;
 	  case 'p':
-	    /* "corearg" is shared by "--core" and "--pid" */
-	    corearg = optarg;
+	    pidarg = optarg;
 	    break;
 	  case 'x':
 	    cmdarg[ncmd].type = CMDARG_FILE;
@@ -572,26 +566,34 @@ extern int gdbtk_test (char *);
       }
     else
       {
-	/* OK, that's all the options.  The other arguments are filenames.  */
-	count = 0;
-	for (; optind < argc; optind++)
-	  switch (++count)
-	    {
-	    case 1:
-	      symarg = argv[optind];
-	      execarg = argv[optind];
-	      break;
-	    case 2:
-	      /* The documentation says this can be a "ProcID" as well. 
-	         We will try it as both a corefile and a pid.  */
-	      corearg = argv[optind];
-	      break;
-	    case 3:
-	      fprintf_unfiltered (gdb_stderr,
-				  _("Excess command line arguments ignored. (%s%s)\n"),
-				  argv[optind], (optind == argc - 1) ? "" : " ...");
-	      break;
-	    }
+	/* OK, that's all the options.  */
+
+	/* The first argument, if specified, is the name of the
+	   executable.  */
+	if (optind < argc)
+	  {
+	    symarg = argv[optind];
+	    execarg = argv[optind];
+	    optind++;
+	  }
+
+	/* If the user hasn't already specified a PID or the name of a
+	   core file, then a second optional argument is allowed.  If
+	   present, this argument should be interpreted as either a
+	   PID or a core file, whichever works.  */
+	if (pidarg == NULL && corearg == NULL && optind < argc)
+	  {
+	    pid_or_core_arg = argv[optind];
+	    optind++;
+	  }
+
+	/* Any argument left on the command line is unexpected and
+	   will be ignored.  Inform the user.  */
+	if (optind < argc)
+	  fprintf_unfiltered (gdb_stderr, _("\
+Excess command line arguments ignored. (%s%s)\n"),
+			      argv[optind],
+			      (optind == argc - 1) ? "" : " ...");
       }
     if (batch)
       quiet = 1;
@@ -733,21 +735,31 @@ extern int gdbtk_test (char *);
 	catch_command_errors (symbol_file_add_main, symarg, 0, RETURN_MASK_ALL);
     }
 
+  if (corearg && pidarg)
+    error (_("\
+Can't attach to process and specify a core file at the same time."));
+
   if (corearg != NULL)
-    {
-      /* corearg may be either a corefile or a pid.
-	 If its first character is a digit, try attach first
-	 and then corefile.  Otherwise try corefile first. */
+    catch_command_errors (core_file_command, corearg,
+			  !batch, RETURN_MASK_ALL);
+  else if (pidarg != NULL)
+    catch_command_errors (attach_command, pidarg,
+			  !batch, RETURN_MASK_ALL);
+  else if (pid_or_core_arg)
+    {
+      /* The user specified 'gdb program pid' or gdb program core'.
+	 If pid_or_core_arg's first character is a digit, try attach
+	 first and then corefile.  Otherwise try just corefile.  */
 
-      if (isdigit (corearg[0]))
+      if (isdigit (pid_or_core_arg[0]))
 	{
-	  if (catch_command_errors (attach_command, corearg, 
+	  if (catch_command_errors (attach_command, pid_or_core_arg,
 				    !batch, RETURN_MASK_ALL) == 0)
-	    catch_command_errors (core_file_command, corearg, 
+	    catch_command_errors (core_file_command, pid_or_core_arg,
 				  !batch, RETURN_MASK_ALL);
 	}
-      else /* Can't be a pid, better be a corefile. */
-	catch_command_errors (core_file_command, corearg, 
+      else /* Can't be a pid, better be a corefile.  */
+	catch_command_errors (core_file_command, pid_or_core_arg,
 			      !batch, RETURN_MASK_ALL);
     }
 
Index: src/gdb/doc/gdbint.texinfo
===================================================================
--- src.orig/gdb/doc/gdbint.texinfo	2008-01-04 11:35:30.000000000 +0000
+++ src/gdb/doc/gdbint.texinfo	2008-01-04 11:35:51.000000000 +0000
@@ -2490,13 +2490,6 @@ of a function to be called if @code{SIGW
 Define this to expand into code that will define the function named by
 the expansion of @code{SIGWINCH_HANDLER}.
 
-@item ALIGN_STACK_ON_STARTUP
-@cindex stack alignment
-Define this if your system is of a sort that will crash in
-@code{tgetent} if the stack happens not to be longword-aligned when
-@code{main} is called.  This is a rare situation, but is known to occur
-on several different types of systems.
-
 @item CRLF_SOURCE_FILES
 @cindex DOS text files
 Define this if host files use @code{\r\n} rather than @code{\n} as a

Re: --pid and --core

[Joel Brobecker]

> There's a wrinkle.  The 'int count' variable that was originally
> used in the loop, was also used to do a last resort stack alignment in
> captured_main's entry.
> 
> #if defined (ALIGN_STACK_ON_STARTUP)
>      i = (int) &count & 0x03;
>      if (i != 0)
>        alloca (4 - i);
> #endif
> 
> ALIGN_STACK_ON_STARTUP isn't defined in any target/host
> anymore, and if it was, count is in the middle of the stack frame
> for long and nobody complained.  We can safely
> remove it.

I agree.

> 2008-01-03  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* main.c (captured_main): Remove 'count' varible and the
> 	ALIGN_STACK_ON_ENTRY block that used it.  Error out if --core and
> 	--pid options were issued simultaneously.  If an explicit pid
> 	option was passed, don't fallback to core file.  Detect extra
> 	arguments better in the presence of explicit pid or core
> 	arguments.

This part is OK, please go ahead and commit.

Thanks,
-- 
Joel

Re: --pid and --core

[Eli Zaretskii]

> Date: Fri, 04 Jan 2008 12:18:01 +0000
> From: Pedro Alves <pedro@codesourcery.com>
> CC:  gdb-patches@sourceware.org
> 
> Eli, gdbint patch OK ?

Yes, thanks.

Re: --pid and --core

[Eli Zaretskii]

> Date: Thu, 03 Jan 2008 16:49:46 +0000
> From: Pedro Alves <pedro@codesourcery.com>
> 
> If the user specifies,
> 
>       gdb program --pid=3333,
> 
> gdb has no business trying to look for a core file named 3333.
> 
> The patch avoids these confusing messages [1] when the user specifies
> an invalid or non-existing pid:

Thanks, but please make the appropriate changes in the manual as
well.  It currently says:

    `-pid NUMBER'
    `-p NUMBER'
	 Connect to process ID NUMBER, as with the `attach' command.  If
	 there is no such process, GDB will attempt to open a core file
	 named NUMBER.

Re: --pid and --core

[Eli Zaretskii]

> Date: Sat, 05 Jan 2008 14:46:05 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> Thanks, but please make the appropriate changes in the manual as
> well.  It currently says:
> 
>     `-pid NUMBER'
>     `-p NUMBER'
> 	 Connect to process ID NUMBER, as with the `attach' command.  If
> 	 there is no such process, GDB will attempt to open a core file
> 	 named NUMBER.

In addition, this change also warrants a NEWS entry.

Re: --pid and --core

[Pedro Alves]

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

Eli Zaretskii wrote:
>> Thanks, but please make the appropriate changes in the manual as
>> well.  It currently says:
>>
>>     `-pid NUMBER'
>>     `-p NUMBER'
>> 	 Connect to process ID NUMBER, as with the `attach' command.  If
>> 	 there is no such process, GDB will attempt to open a core file
>> 	 named NUMBER.
> 
> In addition, this change also warrants a NEWS entry.
> 

Like so ?

-- 
Pedro Alves




[-- Attachment #2: pid_core_doc.diff --]
[-- Type: text/x-diff, Size: 1789 bytes --]

2008-01-05  Pedro Alves  <pedro@codesourcery.com>

	gdb/

	* NEWS: Mention --pid and --core command line behaviour changes.

2008-01-05  Pedro Alves  <pedro@codesourcery.com>

	gdb/doc/

	* gdb.texinfo (File Options): Remove mention of the attempt to
	open a core file with the -p option.  Don't list -c as a valid
	option to attach to a process.

---
 gdb/NEWS            |    7 +++++++
 gdb/doc/gdb.texinfo |    3 ---
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS	2008-01-05 17:22:19.000000000 +0000
+++ src/gdb/NEWS	2008-01-05 17:24:16.000000000 +0000
@@ -3,6 +3,13 @@
 
 *** Changes since GDB 6.7
 
+* Change in command line behavior -- corefiles vs. process ids.
+
+  When the '-p NUMBER' or '--pid NUMBER' options are used, and
+  attaching to process NUMBER fails, GDB no longer attempts to open a
+  core file named NUMBER.  Attaching to a program using the -c option
+  is no longer supported.  Instead, use the '-p' or '--pid' options.
+
 * New commands
 
 set print frame-arguments (all|scalars|none)
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2008-01-05 17:22:19.000000000 +0000
+++ src/gdb/doc/gdb.texinfo	2008-01-05 17:23:56.000000000 +0000
@@ -942,14 +942,11 @@ file.
 @cindex @code{-c}
 Use file @var{file} as a core dump to examine.
 
-@item -c @var{number}
 @item -pid @var{number}
 @itemx -p @var{number}
 @cindex @code{--pid}
 @cindex @code{-p}
 Connect to process ID @var{number}, as with the @code{attach} command.
-If there is no such process, @value{GDBN} will attempt to open a core
-file named @var{number}.
 
 @item -command @var{file}
 @itemx -x @var{file}

Re: --pid and --core

[Eli Zaretskii]

> Date: Sat, 05 Jan 2008 17:25:32 +0000
> From: Pedro Alves <pedro_alves@portugalmail.pt>
> CC: gdb-patches@sourceware.org
> 
> Eli Zaretskii wrote:
> >> Thanks, but please make the appropriate changes in the manual as
> >> well.  It currently says:
> >>
> >>     `-pid NUMBER'
> >>     `-p NUMBER'
> >> 	 Connect to process ID NUMBER, as with the `attach' command.  If
> >> 	 there is no such process, GDB will attempt to open a core file
> >> 	 named NUMBER.
> > 
> > In addition, this change also warrants a NEWS entry.
> > 
> 
> Like so ?

Yes, thanks.

Re: --pid and --core

[Pedro Alves]

Thanks guys.  All check in now.

-- 
Pedro Alves