PATCH RFA: Improve -Wcast-qual warning for C

[Ian Lance Taylor <iant at google dot com>]

As discussed at http://gcc.gnu.org/ml/gcc/2009-05/msg00533.html , the
C++ compiler issues more warnings for -Wcast-qual than the C compiler
does.  After a bit of further discussion on IRC, I think the general
consensus is that the C compiler should issue warnings about unsafe
casts much as the C++ compiler does.  Recall that this is only when
using -Wcast-qual, and that -Wcast-qual is not part of -Wall.

The first patch in this message implements the appropriate warnings.
This patch requires approval from the C frontend maintainers.

The second patch in this message fixes gcc to bootstrap given the
changes in the first patch.  I believe that these changes count as
obvious.

Bootstrapped and tested on i686-pc-linux-gnu.

OK for mainline?

Ian


First patch:

gcc/ChangeLog:

2009-05-25  Ian Lance Taylor  <iant@google.com>

	* c-typeck.c (handle_warn_cast_qual): New static function,
	partially broken out of build_c_cast.
	(build_c_cast): Call handle_warn_cast_qual.
	* doc/invoke.texi (Warning Options): Document new effect of
	-Wcast-qual.

gcc/testsuite/ChangeLog:

2009-05-25  Ian Lance Taylor  <iant@google.com>

	* gcc.dg/cast-qual-3.c: New testcase.


Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 147842)
+++ c-typeck.c	(working copy)
@@ -4055,6 +4055,93 @@ build_compound_expr (tree expr1, tree ex
   return ret;
 }
 
+/* Issue -Wcast-qual warnings when appropriate.  TYPE is the type to
+   which we are casting.  OTYPE is the type of the expression being
+   cast.  Both TYPE and OTYPE are pointer types.  -Wcast-qual appeared
+   on the command line.  */
+
+static void
+handle_warn_cast_qual (tree type, tree otype)
+{
+  tree in_type = type;
+  tree in_otype = otype;
+  int added = 0;
+  int discarded = 0;
+  unsigned int quals;
+
+  /* Check that the qualifiers on IN_TYPE are a superset of the
+     qualifiers of IN_OTYPE.  The outermost level of POINTER_TYPE
+     nodes is uninteresting and we stop as soon as we hit a
+     non-POINTER_TYPE node on either type.  */
+  do
+    {
+      in_otype = TREE_TYPE (in_otype);
+      in_type = TREE_TYPE (in_type);
+
+      /* GNU C allows cv-qualified function types.  'const' means the
+	 function is very pure, 'volatile' means it can't return.  We
+	 need to warn when such qualifiers are added, not when they're
+	 taken away.  */
+      if (TREE_CODE (in_otype) == FUNCTION_TYPE
+	  && TREE_CODE (in_type) == FUNCTION_TYPE)
+	added |= (TYPE_QUALS (in_type) & ~TYPE_QUALS (in_otype));
+      else
+	discarded |= (TYPE_QUALS (in_otype) & ~TYPE_QUALS (in_type));
+    }
+  while (TREE_CODE (in_type) == POINTER_TYPE
+	 && TREE_CODE (in_otype) == POINTER_TYPE);
+
+  if (added)
+    warning (OPT_Wcast_qual, "cast adds new qualifiers to function type");
+
+  if (discarded)
+    /* There are qualifiers present in IN_OTYPE that are not present
+       in IN_TYPE.  */
+    warning (OPT_Wcast_qual,
+	     "cast discards qualifiers from pointer target type");
+
+  if (added || discarded)
+    return;
+
+  /* A cast from **T to const **T is unsafe, because it can cause a
+     value to be changed via a const *T pointer with no additional
+     warning.  We only issue this warning if T is the same on both
+     sides, and we only issue the warning if there is the same number
+     of pointers on both sides, as otherwise the cast is clearly
+     unsafe anyhow.  A cast is unsafe is when a qualifier is added at
+     one level and is not present at all outer levels.
+
+     To issue this warning, we check at each level whether the cast
+     adds new qualifiers not already seen.  We don't need to special
+     case function types, as they won't have the same
+     TYPE_MAIN_VARIANT.  */
+
+  if (TYPE_MAIN_VARIANT (in_type) != TYPE_MAIN_VARIANT (in_otype))
+    return;
+  if (TREE_CODE (TREE_TYPE (type)) != POINTER_TYPE)
+    return;
+
+  in_type = type;
+  in_otype = otype;
+  quals = TYPE_QUALS (TREE_TYPE (in_type));
+  do
+    {
+      unsigned int newquals;
+
+      in_type = TREE_TYPE (in_type);
+      in_otype = TREE_TYPE (in_otype);
+      newquals = TYPE_QUALS (in_type) &~ TYPE_QUALS (in_otype);
+      if ((newquals & ~quals) != 0)
+	{
+	  warning (OPT_Wcast_qual,
+		   "new qualifiers in middle of multi-level cast are unsafe");
+	  break;
+	}
+      quals &= TYPE_QUALS (in_type);
+    }
+  while (TREE_CODE (in_type) == POINTER_TYPE);
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.  */
 
 tree
@@ -4139,46 +4226,10 @@ build_c_cast (tree type, tree expr)
       otype = TREE_TYPE (value);
 
       /* Optionally warn about potentially worrisome casts.  */
-
       if (warn_cast_qual
 	  && TREE_CODE (type) == POINTER_TYPE
 	  && TREE_CODE (otype) == POINTER_TYPE)
-	{
-	  tree in_type = type;
-	  tree in_otype = otype;
-	  int added = 0;
-	  int discarded = 0;
-
-	  /* Check that the qualifiers on IN_TYPE are a superset of
-	     the qualifiers of IN_OTYPE.  The outermost level of
-	     POINTER_TYPE nodes is uninteresting and we stop as soon
-	     as we hit a non-POINTER_TYPE node on either type.  */
-	  do
-	    {
-	      in_otype = TREE_TYPE (in_otype);
-	      in_type = TREE_TYPE (in_type);
-
-	      /* GNU C allows cv-qualified function types.  'const'
-		 means the function is very pure, 'volatile' means it
-		 can't return.  We need to warn when such qualifiers
-		 are added, not when they're taken away.  */
-	      if (TREE_CODE (in_otype) == FUNCTION_TYPE
-		  && TREE_CODE (in_type) == FUNCTION_TYPE)
-		added |= (TYPE_QUALS (in_type) & ~TYPE_QUALS (in_otype));
-	      else
-		discarded |= (TYPE_QUALS (in_otype) & ~TYPE_QUALS (in_type));
-	    }
-	  while (TREE_CODE (in_type) == POINTER_TYPE
-		 && TREE_CODE (in_otype) == POINTER_TYPE);
-
-	  if (added)
-	    warning (OPT_Wcast_qual, "cast adds new qualifiers to function type");
-
-	  if (discarded)
-	    /* There are qualifiers present in IN_OTYPE that are not
-	       present in IN_TYPE.  */
-	    warning (OPT_Wcast_qual, "cast discards qualifiers from pointer target type");
-	}
+	handle_warn_cast_qual (type, otype);
 
       /* Warn about possible alignment problems.  */
       if (STRICT_ALIGNMENT
Index: testsuite/gcc.dg/cast-qual-3.c
===================================================================
--- testsuite/gcc.dg/cast-qual-3.c	(revision 0)
+++ testsuite/gcc.dg/cast-qual-3.c	(revision 0)
@@ -0,0 +1,162 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-qual" } */
+
+void
+f1 (void *bar)
+{
+  const void *p1 = (const void *) bar;
+  const char *p2 = (const char *) bar;
+  const void **p3 = (const void **) bar;
+  const char **p4 = (const char **) bar;
+  const void * const *p5 = (const void * const *) bar;
+  const char * const *p6 = (const char * const *) bar;
+  void * const *p7 = (void * const *) bar;
+  char * const *p8 = (char * const *) bar;
+  const void ***p9 = (const void ***) bar;
+  const char ***p10 = (const char ***) bar;
+  void * const **p11 = (void * const **) bar;
+  char * const **p12 = (char * const **) bar;
+  void ** const *p13 = (void ** const *) bar;
+  char ** const *p14 = (char ** const *) bar;
+  const void * const **p15 = (const void * const **) bar;
+  const char * const **p16 = (const char * const **) bar;
+  const void ** const *p17 = (const void ** const *) bar;
+  const char ** const *p18 = (const char ** const *) bar;
+  void * const * const * p19 = (void * const * const *) bar;
+  char * const * const * p20 = (char * const * const *) bar;
+  const void * const * const *p21 = (const void * const * const *) bar;
+  const char * const * const *p22 = (const char * const * const *) bar;
+}
+
+void
+f2 (void **bar)
+{
+  const void *p1 = (const void *) bar;
+  const char *p2 = (const char *) bar;
+  const void **p3 = (const void **) bar; /* { dg-warning "unsafe" } */
+  const char **p4 = (const char **) bar;
+  const void * const *p5 = (const void * const *) bar;
+  const char * const *p6 = (const char * const *) bar;
+  void * const *p7 = (void * const *) bar;
+  char * const *p8 = (char * const *) bar;
+  const void ***p9 = (const void ***) bar;
+  const char ***p10 = (const char ***) bar;
+  void * const **p11 = (void * const **) bar;
+  char * const **p12 = (char * const **) bar;
+  void ** const *p13 = (void ** const *) bar;
+  char ** const *p14 = (char ** const *) bar;
+  const void * const **p15 = (const void * const **) bar;
+  const char * const **p16 = (const char * const **) bar;
+  const void ** const *p17 = (const void ** const *) bar;
+  const char ** const *p18 = (const char ** const *) bar;
+  void * const * const * p19 = (void * const * const *) bar;
+  char * const * const * p20 = (char * const * const *) bar;
+  const void * const * const *p21 = (const void * const * const *) bar;
+  const char * const * const *p22 = (const char * const * const *) bar;
+}
+
+void
+f3 (void ***bar)
+{
+  const void *p1 = (const void *) bar;
+  const char *p2 = (const char *) bar;
+  const void **p3 = (const void **) bar;
+  const char **p4 = (const char **) bar;
+  const void * const *p5 = (const void * const *) bar;
+  const char * const *p6 = (const char * const *) bar;
+  void * const *p7 = (void * const *) bar;
+  char * const *p8 = (char * const *) bar;
+  const void ***p9 = (const void ***) bar; /* { dg-warning "unsafe" } */
+  const char ***p10 = (const char ***) bar;
+  void * const **p11 = (void * const **) bar; /* { dg-warning "unsafe" } */
+  char * const **p12 = (char * const **) bar;
+  void ** const *p13 = (void ** const *) bar;
+  char ** const *p14 = (char ** const *) bar;
+  const void * const **p15 = (const void * const **) bar; /* { dg-warning "unsafe" } */
+  const char * const **p16 = (const char * const **) bar;
+  const void ** const *p17 = (const void ** const *) bar; /* { dg-warning "unsafe" } */
+  const char ** const *p18 = (const char ** const *) bar;
+  void * const * const * p19 = (void * const * const *) bar;
+  char * const * const * p20 = (char * const * const *) bar;
+  const void * const * const *p21 = (const void * const * const *) bar;
+  const char * const * const *p22 = (const char * const * const *) bar;
+}
+
+void
+f4 (void * const **bar)
+{
+  const void ***p9 = (const void ***) bar; /* { dg-warning "discards qualifier" } */
+  void * const **p11 = (void * const **) bar;
+  void ** const *p13 = (void ** const *) bar; /* { dg-warning "discards qualifier" } */
+  const void * const **p15 = (const void * const **) bar; /* { dg-warning "unsafe" } */
+  const void ** const *p17 = (const void ** const *) bar; /* { dg-warning "discards qualifier" } */
+  void * const * const * p19 = (void * const * const *) bar;
+  const void * const * const *p21 = (const void * const * const *) bar;
+}
+
+void
+f5 (char ***bar)
+{
+  volatile const char ***p9 = (volatile const char ***) bar; /* { dg-warning "unsafe" } */
+  volatile char * const **p11 = (volatile char * const **) bar; /* { dg-warning "unsafe" } */
+  volatile char ** const *p13 = (volatile char ** const *) bar; /* { dg-warning "unsafe" } */
+  volatile const char * const **p15 = (volatile const char * const **) bar; /* { dg-warning "unsafe" } */
+  volatile const char ** const *p17 = (volatile const char ** const *) bar; /* { dg-warning "unsafe" } */
+  volatile char * const * const * p19 = (volatile char * const * const *) bar; /* { dg-warning "unsafe" } */
+  volatile const char * const * const *p21 = (volatile const char * const * const *) bar; /* { dg-warning "unsafe" } */
+}
+
+void
+f6 (char ***bar)
+{
+  const char * volatile **p9 = (const char * volatile **) bar; /* { dg-warning "unsafe" } */
+  char * volatile const **p11 = (char * volatile const **) bar; /* { dg-warning "unsafe" } */
+  char * volatile * const *p13 = (char * volatile * const *) bar; /* { dg-warning "unsafe" } */
+  const char * volatile const **p15 = (const char * volatile const **) bar; /* { dg-warning "unsafe" } */
+  const char * volatile * const *p17 = (const char * volatile * const *) bar; /* { dg-warning "unsafe" } */
+  char * volatile const * const * p19 = (char * volatile const * const *) bar; /* { dg-warning "unsafe" } */
+  const char * volatile const * const *p21 = (const char * volatile const * const *) bar; /* { dg-warning "unsafe" } */
+}
+
+void
+f7 (char ***bar)
+{
+  const char ** volatile *p9 = (const char ** volatile *) bar; /* { dg-warning "unsafe" } */
+  char * const * volatile *p11 = (char * const * volatile *) bar; /* { dg-warning "unsafe" } */
+  char ** volatile const *p13 = (char ** volatile const *) bar;
+  const char * const * volatile *p15 = (const char * const * volatile *) bar; /* { dg-warning "unsafe" } */
+  const char ** volatile const *p17 = (const char ** volatile const *) bar; /* { dg-warning "unsafe" } */
+  char * const * volatile const * p19 = (char * const * volatile const *) bar;
+  const char * const * volatile const *p21 = (const char * const * volatile const *) bar;
+}
+
+typedef int (intfn) (int);
+typedef intfn *pintfn;
+typedef const intfn *constfn;
+
+void
+f8 (constfn ***bar)
+{
+  const constfn *p1 = (const constfn *) bar;
+  const pintfn *p2 = (const pintfn *) bar;
+  const constfn **p3 = (const constfn **) bar;
+  const pintfn **p4 = (const pintfn **) bar;
+  const constfn * const *p5 = (const constfn * const *) bar;
+  const pintfn * const *p6 = (const pintfn * const *) bar;
+  constfn * const *p7 = (constfn * const *) bar;
+  pintfn * const *p8 = (pintfn * const *) bar;
+  const constfn ***p9 = (const constfn ***) bar; /* { dg-warning "unsafe" } */
+  const pintfn ***p10 = (const pintfn ***) bar; /* { dg-warning "unsafe" } */
+  constfn * const **p11 = (constfn * const **) bar; /* { dg-warning "unsafe" } */
+  pintfn * const **p12 = (pintfn * const **) bar; /* { dg-warning "unsafe" } */
+  constfn ** const *p13 = (constfn ** const *) bar;
+  pintfn ** const *p14 = (pintfn ** const *) bar;
+  const constfn * const **p15 = (const constfn * const **) bar; /* { dg-warning "unsafe" } */
+  const pintfn * const **p16 = (const pintfn * const **) bar; /* { dg-warning "unsafe" } */
+  const constfn ** const *p17 = (const constfn ** const *) bar; /* { dg-warning "unsafe" } */
+  const pintfn ** const *p18 = (const pintfn ** const *) bar; /* { dg-warning "unsafe" } */
+  constfn * const * const * p19 = (constfn * const * const *) bar;
+  pintfn * const * const * p20 = (pintfn * const * const *) bar;
+  const constfn * const * const *p21 = (const constfn * const * const *) bar;
+  const pintfn * const * const *p22 = (const pintfn * const * const *) bar;
+}
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 147842)
+++ doc/invoke.texi	(working copy)
@@ -3689,6 +3689,19 @@ Warn whenever a pointer is cast so as to
 the target type.  For example, warn if a @code{const char *} is cast
 to an ordinary @code{char *}.
 
+Also warn when making a cast which introduces a type qualifier in an
+unsafe way.  For example, casting @code{char **} to @code{const char **}
+is unsafe, as in this example:
+
+@smallexample
+  /* p is char ** value.  */
+  const char **q = (const char **) p;
+  /* Assignment of readonly string to const char * is OK.  */
+  *q = "string";
+  /* Now char** pointer points to read-only memory.  */
+  **p = 'b';
+@end smallexample
+
 @item -Wcast-align
 @opindex Wcast-align
 @opindex Wno-cast-align


==================================================


Second patch:

gcc/ChangeLog:

2009-05-25  Ian Lance Taylor  <iant@google.com>

	* attribs.c (register_attribute): Use CONST_CAST.
	* collect2.c (main): Use CONST_CAST2.
	(scan_prog_file): Likewise.
	* gcc.c (process_command, main): Likewise.
	* toplev.c (toplev_main): Likewise.

gcc/java/ChangeLog:

2009-05-25  Ian Lance Taylor  <iant@google.com>

	* jcf-io.c (find_class): Use CONST_CAST.


Index: attribs.c
===================================================================
--- attribs.c	(revision 147842)
+++ attribs.c	(working copy)
@@ -1,6 +1,6 @@
 /* Functions dealing with attribute handling, used by most front ends.
    Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001,
-   2002, 2003, 2004, 2005, 2007, 2008 Free Software Foundation, Inc.
+   2002, 2003, 2004, 2005, 2007, 2008, 2009 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -195,15 +195,15 @@ void
 register_attribute (const struct attribute_spec *attr) 
 {
   struct substring str;
-  const void **slot;
+  void **slot;
 
   str.str = attr->name;
   str.length = strlen (str.str);
-  slot = (const void **)htab_find_slot_with_hash (attribute_hash, &str,
-                                                  substring_hash (str.str, str.length),
-                                                  INSERT);
+  slot = htab_find_slot_with_hash (attribute_hash, &str,
+				   substring_hash (str.str, str.length),
+				   INSERT);
   gcc_assert (!*slot);
-  *slot = attr;
+  *slot = (void *) CONST_CAST (struct attribute_spec *, attr);
 }
 
 /* Return the spec for the attribute named NAME.  */
Index: java/jcf-io.c
===================================================================
--- java/jcf-io.c	(revision 147842)
+++ java/jcf-io.c	(working copy)
@@ -1,6 +1,6 @@
 /* Utility routines for finding and reading Java(TM) .class files.
    Copyright (C) 1996, 1997, 1998, 1999, 2000, 2002, 2003, 2004, 2005,
-   2006, 2007, 2008 Free Software Foundation, Inc.
+   2006, 2007, 2008, 2009 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -399,9 +399,8 @@ find_class (const char *classname, int c
 
   /* Remember that this class could not be found so that we do not
      have to look again.  */
-  *(const void **)htab_find_slot_with_hash (memoized_class_lookups,
-					    classname, hash, INSERT)
-    = classname;
+  *htab_find_slot_with_hash (memoized_class_lookups, classname, hash, INSERT)
+    = (void *) CONST_CAST (char *, classname);
 
   return NULL;
  found:
Index: gcc.c
===================================================================
--- gcc.c	(revision 147842)
+++ gcc.c	(working copy)
@@ -3636,10 +3636,15 @@ process_command (int argc, const char **
     }
 
   /* Convert new-style -- options to old-style.  */
-  translate_options (&argc, (const char *const **) &argv);
+  translate_options (&argc,
+		     CONST_CAST2 (const char *const **, const char ***,
+				  &argv));
 
   /* Do language-specific adjustment/addition of flags.  */
-  lang_specific_driver (&argc, (const char *const **) &argv, &added_libraries);
+  lang_specific_driver (&argc,
+			CONST_CAST2 (const char *const **, const char ***,
+				     &argv),
+			&added_libraries);
 
   /* Scan argv twice.  Here, the first time, just count how many switches
      there will be in their vector, and how many input files in theirs.
@@ -6466,7 +6471,7 @@ main (int argc, char **argv)
      Make a table of specified input files (infiles, n_infiles).
      Decode switches that are handled locally.  */
 
-  process_command (argc, (const char **) argv);
+  process_command (argc, CONST_CAST2 (const char **, char **, argv));
 
   /* Initialize the vector of specs to just the default.
      This means one element containing 0s, as a terminator.  */
Index: toplev.c
===================================================================
--- toplev.c	(revision 147842)
+++ toplev.c	(working copy)
@@ -1,6 +1,6 @@
 /* Top level of GCC compilers (cc1, cc1plus, etc.)
    Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -2349,14 +2349,14 @@ toplev_main (int argc, char **argv)
 {
   expandargv (&argc, &argv);
 
-  save_argv = (const char **) argv;
+  save_argv = CONST_CAST2 (const char **, char **, argv);
 
   /* Initialization of GCC's environment, and diagnostics.  */
   general_init (argv[0]);
 
   /* Parse the options and do minimal processing; basically just
      enough to default flags appropriately.  */
-  decode_options (argc, (const char **) argv);
+  decode_options (argc, CONST_CAST2 (const char **, char **, argv));
 
   init_local_tick ();
 
Index: collect2.c
===================================================================
--- collect2.c	(revision 147842)
+++ collect2.c	(working copy)
@@ -857,9 +857,12 @@ main (int argc, char **argv)
   /* Do not invoke xcalloc before this point, since locale needs to be
      set first, in case a diagnostic is issued.  */
 
-  ld1 = (const char **)(ld1_argv = XCNEWVEC (char *, argc+4));
-  ld2 = (const char **)(ld2_argv = XCNEWVEC (char *, argc+11));
-  object = (const char **)(object_lst = XCNEWVEC (char *, argc));
+  ld1_argv = XCNEWVEC (char *, argc + 4);
+  ld1 = CONST_CAST2 (const char **, char **, ld1_argv);
+  ld2_argv = XCNEWVEC (char *, argc + 11);
+  ld2 = CONST_CAST2 (const char **, char **, ld2_argv);
+  object_lst = XCNEWVEC (char *, argc);
+  object = CONST_CAST2 (const char **, char **, object_lst);
 
 #ifdef DEBUG
   debug = 1;
@@ -904,7 +907,8 @@ main (int argc, char **argv)
      -fno-exceptions -w */
   num_c_args += 5;
 
-  c_ptr = (const char **) (c_argv = XCNEWVEC (char *, num_c_args));
+  c_argv = XCNEWVEC (char *, num_c_args);
+  c_ptr = CONST_CAST2 (const char **, char **, c_argv);
 
   if (argc < 2)
     fatal ("no arguments");
@@ -1406,7 +1410,8 @@ main (int argc, char **argv)
       if (strip_flag)
 	{
 	  char **real_strip_argv = XCNEWVEC (char *, 3);
-	  const char ** strip_argv = (const char **) real_strip_argv;
+	  const char ** strip_argv = CONST_CAST2 (const char **, char **,
+						  real_strip_argv);
 
 	  strip_argv[0] = strip_file_name;
 	  strip_argv[1] = output_file;
@@ -2090,7 +2095,7 @@ scan_prog_file (const char *prog_name, e
   void (*quit_handler) (int);
 #endif
   char *real_nm_argv[4];
-  const char **nm_argv = (const char **) real_nm_argv;
+  const char **nm_argv = CONST_CAST2 (const char **, char**, real_nm_argv);
   int argc = 0;
   struct pex_obj *pex;
   const char *errmsg;