[patch] Remove one immediate_quit++-- (in auto-load.c)

[patch] Remove one immediate_quit++-- (in auto-load.c)

[Jan Kratochvil]

Hi Doug,

Tom has notified me on #gdb there was one needless and probably even incorrect
immediate_quit++ (and --).

despite it is in auto-load.c it was only moved there with other code, it
originates from patch:

commit 47d11be572aaf42cc67e6d925271d44bdecd104d
Author: Doug Evans <dje@google.com>
Date:   Fri Apr 23 16:20:08 2010 +0000
	Add support for auto-loading scripts from .debug_gdb_scripts section.

Originally there was printf_filtered which does not need it.  Later it is just
around code with no UI output:

commit 2e0e11bc91732db59fe04fca4f2dceff2bb4a2ad
Author: Doug Evans <dje@google.com>
Date:   Fri May 13 22:11:45 2011 +0000

I will check it in.


Thanks,
Jan


gdb/
2012-07-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* auto-load.c (auto_load_info_scripts): Remove immediate_quit increment
	and decrement.

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 87dd1e4..2cc52c6 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -916,11 +916,9 @@ auto_load_info_scripts (char *pattern, int from_tty,
     {
       struct collect_matching_scripts_data data = { &scripts, language };
 
-      immediate_quit++;
       /* Pass a pointer to scripts as VEC_safe_push can realloc space.  */
       htab_traverse_noresize (pspace_info->loaded_scripts,
 			      collect_matching_scripts, &data);
-      immediate_quit--;
     }
 
   nr_scripts = VEC_length (loaded_script_ptr, scripts);

Re: [patch] Remove one immediate_quit++-- (in auto-load.c)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Tom has notified me on #gdb there was one needless and probably
Jan> even incorrect immediate_quit++ (and --).

I audited all the modifications of immediate_quit.  Many of them are
bogus.

immediate_quit is hard to use reliably.  I think it should only really
be done around I/O.

I'm checking in this patch.  It either removes uses of immediate_quit or
replaces them with QUIT.


Note that there are still some bad uses:

* remote_start_remote increments it but:

  1. Does a non-trivial amount of work before decrementing it.
     It is hard to believe that all this work is really
     immediate-quit-safe.

  2. AFAICT, never decrements it along the non-stop path.
     This seems like it could cause many kinds of bugs.

  3. Calls 'error' with immediate_quit still incremented.

* command_line_input and prompt_for_continue both do quite a bit of work
  with immediate_quit non-zero.  I'm not confident about the results.

I didn't try touching these.

Tom

2012-07-26  Tom Tromey  <tromey@redhat.com>

	* symmisc.c (print_symbol_bcache_statistics): Use QUIT, not
	immediate_quit.
	(print_objfile_statistics): Likewise.
	(maintenance_print_symbols): Likewise.
	(maintenance_print_msymbols): Likewise.
	(maintenance_print_objfiles): Likewise.
	* psymtab.c (print_partial_symbols): Call QUIT.
	(maintenance_print_psymbols): Likewise.  Don't modify
	immediate_quit.
	* copying.c (show_copying_command): Don't modify immediate_quit.
	(show_warranty_command): Likewise.
	* cli/cli-cmds.c (show_version): Don't modify immediate_quit.

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 24d55c3..f2328e8 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -319,10 +319,8 @@ is_complete_command (struct cmd_list_element *c)
 static void
 show_version (char *args, int from_tty)
 {
-  immediate_quit++;
   print_gdb_version (gdb_stdout);
   printf_filtered ("\n");
-  immediate_quit--;
 }
 
 /* Handle the quit command.  */
diff --git a/gdb/copying.c b/gdb/copying.c
index d608a63..9aac2e6 100644
--- a/gdb/copying.c
+++ b/gdb/copying.c
@@ -11,15 +11,13 @@ static void show_warranty_command (char *, int);
 
 void _initialize_copying (void);
 
-extern int immediate_quit;
 static void
 show_copying_command (char *ignore, int from_tty)
 {
-  immediate_quit++;
   printf_filtered ("                    GNU GENERAL PUBLIC LICENSE\n");
   printf_filtered ("                       Version 3, 29 June 2007\n");
   printf_filtered ("\n");
-  printf_filtered (" Copyright (C) 2007 Free Software Foundation, Inc. <http://fsf.org/>\n");
+  printf_filtered (" Copyright (C) 2007, 2012 Free Software Foundation, Inc. <http://fsf.org/>\n");
   printf_filtered (" Everyone is permitted to copy and distribute verbatim copies\n");
   printf_filtered (" of this license document, but changing it is not allowed.\n");
   printf_filtered ("\n");
@@ -604,13 +602,11 @@ show_copying_command (char *ignore, int from_tty)
   printf_filtered ("author or copyright holder as a result of your choosing to follow a\n");
   printf_filtered ("later version.\n");
   printf_filtered ("\n");
-  immediate_quit--;
 }
 
 static void
 show_warranty_command (char *ignore, int from_tty)
 {
-  immediate_quit++;
   printf_filtered ("  15. Disclaimer of Warranty.\n");
   printf_filtered ("\n");
   printf_filtered ("  THERE IS NO WARRANTY FOR THE PROGRAM, TO THE EXTENT PERMITTED BY\n");
@@ -643,7 +639,6 @@ show_warranty_command (char *ignore, int from_tty)
   printf_filtered ("Program, unless a warranty or assumption of liability accompanies a\n");
   printf_filtered ("copy of the Program in return for a fee.\n");
   printf_filtered ("\n");
-  immediate_quit--;
 }
 
 void
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 5623e2d..93be085 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -888,6 +888,7 @@ print_partial_symbols (struct gdbarch *gdbarch,
   fprintf_filtered (outfile, "  %s partial symbols:\n", what);
   while (count-- > 0)
     {
+      QUIT;
       fprintf_filtered (outfile, "    `%s'", SYMBOL_LINKAGE_NAME (*p));
       if (SYMBOL_DEMANGLED_NAME (*p) != NULL)
 	{
@@ -1852,11 +1853,12 @@ print-psymbols takes an output file name and optional symbol file name"));
     perror_with_name (filename);
   make_cleanup_ui_file_delete (outfile);
 
-  immediate_quit++;
   ALL_PSYMTABS (objfile, ps)
-    if (symname == NULL || filename_cmp (symname, ps->filename) == 0)
-    dump_psymtab (objfile, ps, outfile);
-  immediate_quit--;
+    {
+      QUIT;
+      if (symname == NULL || filename_cmp (symname, ps->filename) == 0)
+	dump_psymtab (objfile, ps, outfile);
+    }
   do_cleanups (cleanups);
 }
 
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index d5a737b..d3028e6 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -85,17 +85,16 @@ print_symbol_bcache_statistics (void)
   struct program_space *pspace;
   struct objfile *objfile;
 
-  immediate_quit++;
   ALL_PSPACES (pspace)
     ALL_PSPACE_OBJFILES (pspace, objfile)
   {
+    QUIT;
     printf_filtered (_("Byte cache statistics for '%s':\n"), objfile->name);
     print_bcache_statistics (psymbol_bcache_get_bcache (objfile->psymbol_cache),
                              "partial symbol cache");
     print_bcache_statistics (objfile->macro_cache, "preprocessor macro cache");
     print_bcache_statistics (objfile->filename_cache, "file name cache");
   }
-  immediate_quit--;
 }
 
 void
@@ -106,10 +105,10 @@ print_objfile_statistics (void)
   struct symtab *s;
   int i, linetables, blockvectors;
 
-  immediate_quit++;
   ALL_PSPACES (pspace)
     ALL_PSPACE_OBJFILES (pspace, objfile)
   {
+    QUIT;
     printf_filtered (_("Statistics for '%s':\n"), objfile->name);
     if (OBJSTAT (objfile, n_stabs) > 0)
       printf_filtered (_("  Number of \"stab\" symbols read: %d\n"),
@@ -156,7 +155,6 @@ print_objfile_statistics (void)
     printf_filtered (_("  Total memory used for file name cache: %d\n"),
 		     bcache_memory_used (objfile->filename_cache));
   }
-  immediate_quit--;
 }
 
 static void
@@ -437,11 +435,12 @@ maintenance_print_symbols (char *args, int from_tty)
     perror_with_name (filename);
   make_cleanup_ui_file_delete (outfile);
 
-  immediate_quit++;
   ALL_SYMTABS (objfile, s)
-    if (symname == NULL || filename_cmp (symname, s->filename) == 0)
-    dump_symtab (objfile, s, outfile);
-  immediate_quit--;
+    {
+      QUIT;
+      if (symname == NULL || filename_cmp (symname, s->filename) == 0)
+	dump_symtab (objfile, s, outfile);
+    }
   do_cleanups (cleanups);
 }
 
@@ -663,13 +662,14 @@ maintenance_print_msymbols (char *args, int from_tty)
     perror_with_name (filename);
   make_cleanup_ui_file_delete (outfile);
 
-  immediate_quit++;
   ALL_PSPACES (pspace)
     ALL_PSPACE_OBJFILES (pspace, objfile)
-      if (symname == NULL || (!stat (objfile->name, &obj_st)
-			      && sym_st.st_ino == obj_st.st_ino))
-	dump_msymbols (objfile, outfile);
-  immediate_quit--;
+      {
+	QUIT;
+	if (symname == NULL || (!stat (objfile->name, &obj_st)
+				&& sym_st.st_ino == obj_st.st_ino))
+	  dump_msymbols (objfile, outfile);
+      }
   fprintf_filtered (outfile, "\n\n");
   do_cleanups (cleanups);
 }
@@ -682,11 +682,12 @@ maintenance_print_objfiles (char *ignore, int from_tty)
 
   dont_repeat ();
 
-  immediate_quit++;
   ALL_PSPACES (pspace)
     ALL_PSPACE_OBJFILES (pspace, objfile)
-      dump_objfile (objfile);
-  immediate_quit--;
+      {
+	QUIT;
+	dump_objfile (objfile);
+      }
 }
 
 

[commit] [patch] Remove one immediate_quit++-- (in auto-load.c)

[Jan Kratochvil]

On Thu, 26 Jul 2012 15:54:46 +0200, Jan Kratochvil wrote:
> gdb/
> 2012-07-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* auto-load.c (auto_load_info_scripts): Remove immediate_quit increment
> 	and decrement.

When Tom already checked the other similar patches I also have done so:
	http://sourceware.org/ml/gdb-cvs/2012-07/msg00221.html


Thanks,
Jan

Re: [patch] Remove one immediate_quit++-- (in auto-load.c)

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> -  printf_filtered (" Copyright (C) 2007 Free Software Foundation, Inc. <http://fsf.org/>\n");
Tom> +  printf_filtered (" Copyright (C) 2007, 2012 Free Software Foundation, Inc. <http://fsf.org/>\n");

Pedro pointed out that I put this in by mistake.  Sigh.
And then, double sigh, I noticed:

/* ==> Do not modify this file!!  It is created automatically
   by copying.awk.  Modify copying.awk instead.  <== */

Then, when fixing this, I noticed that there was a previous change to
copying.c that wasn't first done in copying.awk.  So, I fixed that as
well.

Here's the patch.

Tom

2012-07-26  Tom Tromey  <tromey@redhat.com>

	* copying.c: Rebuild.
	* copying.awk: Don't use immediate_quit.  Use 'no_set_class', not
	'no_class'.

Index: copying.awk
===================================================================
RCS file: /cvs/src/src/gdb/copying.awk,v
retrieving revision 1.4
diff -u -r1.4 copying.awk
--- copying.awk	23 Aug 2007 20:33:48 -0000	1.4
+++ copying.awk	26 Jul 2012 17:19:57 -0000
@@ -13,11 +13,9 @@
 	  print ""
 	  print "void _initialize_copying (void);"
 	  print ""
-	  print "extern int immediate_quit;";
 	  print "static void";
 	  print "show_copying_command (char *ignore, int from_tty)";
 	  print "{";
-	  print "  immediate_quit++;";
 	}
 NR == 1,/^[ 	]*15\. Disclaimer of Warranty\.[ 	]*$/	{
 	  if ($0 ~ /\f/)
@@ -33,13 +31,11 @@
 	    }
 	}
 /^[	 ]*15\. Disclaimer of Warranty\.[ 	]*$/	{
-	  print "  immediate_quit--;";
 	  print "}";
 	  print "";
 	  print "static void";
 	  print "show_warranty_command (char *ignore, int from_tty)";
 	  print "{";
-	  print "  immediate_quit++;";
 	}
 /^[ 	]*15\. Disclaimer of Warranty\.[ 	]*$/, /^[ 	]*END OF TERMS AND CONDITIONS[ 	]*$/{  
 	  if (! ($0 ~ /^[ 	]*END OF TERMS AND CONDITIONS[ 	]*$/)) 
@@ -51,16 +47,15 @@
 	    }
 	}
 END	{
-	  print "  immediate_quit--;";
 	  print "}";
 	  print "";
 	  print "void"
 	  print "_initialize_copying (void)";
 	  print "{";
-	  print "  add_cmd (\"copying\", no_class, show_copying_command,";
+	  print "  add_cmd (\"copying\", no_set_class, show_copying_command,";
 	  print "	   _(\"Conditions for redistributing copies of GDB.\"),";
 	  print "	   &showlist);";
-	  print "  add_cmd (\"warranty\", no_class, show_warranty_command,";
+	  print "  add_cmd (\"warranty\", no_set_class, show_warranty_command,";
 	  print "	   _(\"Various kinds of warranty you do not have.\"),";
 	  print "	   &showlist);";
 	  print "";
Index: copying.c
===================================================================
RCS file: /cvs/src/src/gdb/copying.c,v
retrieving revision 1.8
diff -u -r1.8 copying.c
--- copying.c	26 Jul 2012 16:57:21 -0000	1.8
+++ copying.c	26 Jul 2012 17:19:57 -0000
@@ -17,7 +17,7 @@
   printf_filtered ("                    GNU GENERAL PUBLIC LICENSE\n");
   printf_filtered ("                       Version 3, 29 June 2007\n");
   printf_filtered ("\n");
-  printf_filtered (" Copyright (C) 2007, 2012 Free Software Foundation, Inc. <http://fsf.org/>\n");
+  printf_filtered (" Copyright (C) 2007 Free Software Foundation, Inc. <http://fsf.org/>\n");
   printf_filtered (" Everyone is permitted to copy and distribute verbatim copies\n");
   printf_filtered (" of this license document, but changing it is not allowed.\n");
   printf_filtered ("\n");

Re: [patch] Remove one immediate_quit++-- (in auto-load.c)

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom>   3. Calls 'error' with immediate_quit still incremented.

Oops, I forgot that this is actually ok: throw_exception clears
immediate_quit.  The other things are still problems, though.

Tom