diff mbox series

[v2] c: don't drop typedef information in casts

Message ID 20210312030817.2458143-1-equinox@diac24.net
State New
Headers show
Series [v2] c: don't drop typedef information in casts | expand

Commit Message

David Lamparter March 12, 2021, 3:08 a.m. UTC 
The TYPE_MAIN_VARIANT() here was, for casts to a typedef'd type name,
resulting in all information about the typedef's involvement getting
lost.  This drops necessary information for warnings and can make them
confusing or even misleading.  It also makes specialized warnings for
unspecified-size system types (pid_t, uid_t, ...) impossible.

gcc/c/ChangeLog:
2021-03-09  David Lamparter  <equinox@diac24.net>

	PR c/99526
	* c-typeck.c (build_c_cast): retain (unqualified) typedefs in
	  casts rather than stripping down to basic type.
---
 gcc/c/c-typeck.c                    | 37 ++++++++++++++++++++++++++---
 gcc/testsuite/gcc.dg/cast-typedef.c | 29 ++++++++++++++++++++++
 2 files changed, 63 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cast-typedef.c

Comments

Jakub Jelinek March 12, 2021, 8:35 a.m. UTC | #1 
On Fri, Mar 12, 2021 at 04:08:17AM +0100, David Lamparter wrote:
> 
> The TYPE_MAIN_VARIANT() here was, for casts to a typedef'd type name,
> resulting in all information about the typedef's involvement getting
> lost.  This drops necessary information for warnings and can make them
> confusing or even misleading.  It also makes specialized warnings for
> unspecified-size system types (pid_t, uid_t, ...) impossible.

I don't think you can/should do the lookup_name calls in build_c_cast,
that just can't be right.  The lookups need to be done only when parsing
the typedefs.  Because, one certainly can have e.g.
typedef ... A;
typedef A ... B; // with some extra qualifiers or whatever
void
foo (void)
{
  typedef ... A; // Override in local scope A but not B
  ... = (B) ...; // build_c_cast called
}
You don't want to find the overridden A, you need the original one.

> 2021-03-09  David Lamparter  <equinox@diac24.net>
> 
> 	PR c/99526
> 	* c-typeck.c (build_c_cast): retain (unqualified) typedefs in
> 	  casts rather than stripping down to basic type.

	Jakub
David Lamparter March 15, 2021, 6:14 p.m. UTC | #2 
On Fri, Mar 12, 2021 at 09:35:44AM +0100, Jakub Jelinek wrote:
> On Fri, Mar 12, 2021 at 04:08:17AM +0100, David Lamparter wrote:
> > 
> > The TYPE_MAIN_VARIANT() here was, for casts to a typedef'd type name,
> > resulting in all information about the typedef's involvement getting
> > lost.  This drops necessary information for warnings and can make them
> > confusing or even misleading.  It also makes specialized warnings for
> > unspecified-size system types (pid_t, uid_t, ...) impossible.
> 
> I don't think you can/should do the lookup_name calls in build_c_cast,
> that just can't be right.  The lookups need to be done only when parsing
> the typedefs.

I don't have enough understanding of the GCC codebase to be able to make
a more complex change than this, but your feedback gave me another idea:

> Because, one certainly can have e.g.
> typedef ... A;
> typedef A ... B; // with some extra qualifiers or whatever
> void
> foo (void)
> {
>   typedef ... A; // Override in local scope A but not B
>   ... = (B) ...; // build_c_cast called
> }
> You don't want to find the overridden A, you need the original one.

... I could simply check the lookup result for equality against the type
being iterated over (without its qualifiers).  While this means that
shadowed types like in your example will be skipped, this kiiiinda makes
sense from a user perspective since there wouldn't be a distinguisher in
the report.

Patch following up to this mail.  It still does the lookup in
build_c_cast, do you think this altered approach is acceptable?

Cheers,


-David
diff mbox series

Patch

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 4e6d369c4c9c..7323a8e82547 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -5816,6 +5816,7 @@  c_safe_function_type_cast_p (tree t1, tree t2)
 tree
 build_c_cast (location_t loc, tree type, tree expr)
 {
+  tree res_type, walk_type;
   tree value;
 
   bool int_operands = EXPR_INT_CONST_OPERANDS (expr);
@@ -5836,7 +5837,37 @@  build_c_cast (location_t loc, tree type, tree expr)
   if (objc_is_object_ptr (type) && objc_is_object_ptr (TREE_TYPE (expr)))
     return build1 (NOP_EXPR, type, expr);
 
+  /* Want to keep typedef information, but at the same time we need to strip
+     qualifiers for a proper rvalue.  Unfortunately, we don't know if any
+     qualifiers on a typedef are part of the typedef or were locally supplied.
+     So grab the original typedef and use that only if it has no qualifiers.
+     cf. cast-typedef testcase */
+
+  res_type = NULL;
+  walk_type = type;
+
+  while (walk_type && TYPE_NAME (walk_type)
+	 && TREE_CODE (TYPE_NAME (walk_type)) == TYPE_DECL)
+    {
+      tree orig_decl = lookup_name (TYPE_IDENTIFIER (walk_type));
+
+      if (!orig_decl || TREE_CODE (orig_decl) != TYPE_DECL)
+	break;
+
+      walk_type = TREE_TYPE (orig_decl);
+      if (TYPE_QUALS (walk_type) == TYPE_UNQUALIFIED)
+	{
+	  res_type = walk_type;
+	  break;
+	}
+
+      walk_type = DECL_ORIGINAL_TYPE (TYPE_NAME (walk_type));
+    }
+
   type = TYPE_MAIN_VARIANT (type);
+  if (!res_type)
+    res_type = type;
+  gcc_assert (TYPE_MAIN_VARIANT (res_type) == type);
 
   if (TREE_CODE (type) == ARRAY_TYPE)
     {
@@ -5864,7 +5895,7 @@  build_c_cast (location_t loc, tree type, tree expr)
 		 "ISO C forbids casting nonscalar to the same type");
 
       /* Convert to remove any qualifiers from VALUE's type.  */
-      value = convert (type, value);
+      value = convert (res_type, value);
     }
   else if (TREE_CODE (type) == UNION_TYPE)
     {
@@ -6018,7 +6049,7 @@  build_c_cast (location_t loc, tree type, tree expr)
 		    " from %qT to %qT", otype, type);
 
       ovalue = value;
-      value = convert (type, value);
+      value = convert (res_type, value);
 
       /* Ignore any integer overflow caused by the cast.  */
       if (TREE_CODE (value) == INTEGER_CST && !FLOAT_TYPE_P (otype))
@@ -6054,7 +6085,7 @@  build_c_cast (location_t loc, tree type, tree expr)
 		&& INTEGRAL_TYPE_P (TREE_TYPE (expr)))
 	       || TREE_CODE (expr) == REAL_CST
 	       || TREE_CODE (expr) == COMPLEX_CST)))
-      value = build1 (NOP_EXPR, type, value);
+      value = build1 (NOP_EXPR, res_type, value);
 
   /* If the expression has integer operands and so can occur in an
      unevaluated part of an integer constant expression, ensure the
diff --git a/gcc/testsuite/gcc.dg/cast-typedef.c b/gcc/testsuite/gcc.dg/cast-typedef.c
new file mode 100644
index 000000000000..488624c16f72
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cast-typedef.c
@@ -0,0 +1,29 @@ 
+/* Test cast <> typedef interactions */
+/* Origin: David Lamparter <equinox@diac24.net> */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion" } */
+
+typedef int typedefname;
+typedef volatile int qual1;
+typedef volatile typedefname qual2;
+
+extern int val;
+extern void f2(unsigned char arg);
+
+void
+f (void)
+{
+  /* -Wconversion just used to print out the actual type */
+
+  f2 ((typedefname) val); /* { dg-warning "typedefname" } */
+  f2 ((volatile typedefname) val); /* { dg-warning "typedefname" } */
+  f2 ((qual1) val); /* { dg-warning "int" } */
+  f2 ((qual2) val); /* { dg-warning "typedefname" } */
+
+  /* { dg-bogus "volatile" "qualifiers should be stripped" { target { "*-*-*" } } 19  } */
+  /* { dg-bogus "volatile" "qualifiers should be stripped" { target { "*-*-*" } } 20  } */
+  /* { dg-bogus "volatile" "qualifiers should be stripped" { target { "*-*-*" } } 21  } */
+
+  /* { dg-bogus "qual1" "typedef with qualifier should not be used" { target { "*-*-*" } } 20  } */
+  /* { dg-bogus "qual2" "typedef with qualifier should not be used" { target { "*-*-*" } } 21  } */
+}