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

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

[David Lamparter]

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


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>

        * c-typeck.c (build_c_cast): retain (unqualified) typedefs in
          casts rather than stripping down to basic type.
---
 gcc/c/c-typeck.c              | 11 +++++++----
 gcc/testsuite/gcc.dg/cast-5.c | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cast-5.c


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-c-don-t-drop-typedef-information-in-casts.patch --]
[-- Type: text/x-patch; name="0001-c-don-t-drop-typedef-information-in-casts.patch", Size: 2669 bytes --]

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 4e6d369c4c9c..0f67753be4d7 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;
   tree value;
 
   bool int_operands = EXPR_INT_CONST_OPERANDS (expr);
@@ -5836,7 +5837,9 @@ 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);
 
-  type = TYPE_MAIN_VARIANT (type);
+  /* rvalue - retain typedefs w/o qualifier, but use actual type for checks */
+  res_type = build_qualified_type (type, TYPE_UNQUALIFIED);
+  type = TYPE_MAIN_VARIANT (res_type);
 
   if (TREE_CODE (type) == ARRAY_TYPE)
     {
@@ -5864,7 +5867,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 +6021,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 +6057,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-5.c b/gcc/testsuite/gcc.dg/cast-5.c
new file mode 100644
index 000000000000..dcd9b290653d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cast-5.c
@@ -0,0 +1,19 @@
+/* Test cast <> typedef interactions */
+/* Origin: David Lamparter <equinox@diac24.net> */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion" } */
+
+typedef int typedefname;
+typedef volatile int qualtypedefname;
+
+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 ((qualtypedefname) val); /* { dg-warning "qualtypedefname" } */ /* { dg-bogus "volatile" } */
+}

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

[David Lamparter]

On Wed, Mar 10, 2021 at 02:28:15AM +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 hope I didn't produce any glaring SNAFUs in submitting this patch, I
haven't previously contributed to GCC.

I've run the testsuite on x86_64 and seen no breakage from this.  The
delta (compared to this run:
https://gcc.gnu.org/pipermail/gcc-testresults/2021-March/662078.html) is:

$ diff -U0 /tmp/before.sum /tmp/after.sum
--- /tmp/before.sum     2021-03-09 22:48:26.989146223 +0100
+++ /tmp/after.sum      2021-03-10 01:46:46.209935875 +0100
@@ -89 +89,2 @@
-FAIL: g++.old-deja/g++.other/virtual2.C  -std=c++2a execution test
+FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (internal compiler error)
+FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (test for excess errors)

None of which seems to be related to the patch.  (The 2 new fails are
"xtreme-header-5_c.C:3:30: internal compiler error: same canonical type node for different types 'void' and 'std::__void_t<typename _Func::is_transparent>'")

Cheers,


David

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

[Martin Sebor]

On 3/9/21 6:46 PM, David Lamparter wrote:
> On Wed, Mar 10, 2021 at 02:28:15AM +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 hope I didn't produce any glaring SNAFUs in submitting this patch, I
> haven't previously contributed to GCC.
> 
> I've run the testsuite on x86_64 and seen no breakage from this.  The
> delta (compared to this run:
> https://gcc.gnu.org/pipermail/gcc-testresults/2021-March/662078.html) is:

Thanks for the patch and for clarifying how you tested the change!

> 
> $ diff -U0 /tmp/before.sum /tmp/after.sum
> --- /tmp/before.sum     2021-03-09 22:48:26.989146223 +0100
> +++ /tmp/after.sum      2021-03-10 01:46:46.209935875 +0100
> @@ -89 +89,2 @@
> -FAIL: g++.old-deja/g++.other/virtual2.C  -std=c++2a execution test
> +FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (internal compiler error)
> +FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (test for excess errors)
> 
> None of which seems to be related to the patch.  (The 2 new fails are
> "xtreme-header-5_c.C:3:30: internal compiler error: same canonical type node for different types 'void' and 'std::__void_t<typename _Func::is_transparent>'")

Yes, these are unrelated C++ failures.

A few comments on the patch.  It took me a bit at first to see
the problem it tries to solve.  I see the improvement but I'm not
sure it's quite correct, or what in the test verifies the desired
outcome.  The warning on line 12 before the change is:

cast-5.c:13:7: warning: conversion from ‘int’ to ‘unsigned char’ may 
change value [-Wconversion]

and after:

cast-5.c:13:7: warning: conversion from ‘qualtypedefname’ {aka ‘int’} to 
‘unsigned char’ may change value [-Wconversion]

There's no volatile, so the test passes either way.  More important,
though, qualtypedefname is a typedef for volatile int but the AKA
type says it's int.  So the new output doesn't seem right.

In any case, the test should fail without the patch.  Using
dg-warning to look for both the expected typedef name and the AKA
type would seem like a better to do that than dg-bogus.

As an aside, although it's not a requirement, it's helpful to open
a bug first, before submitting a patch.  It's more overhead but it
gives the problem more visibility, lets us track regressions and
related bugs, and help decide what fixes should be backported.  In
this case, the same improvement can also be made to the C++ front
end and having a bug might help highlight that.

Finally, GCC is in a regression-fixing stage now, so unless this
problem is one (having a bug report would also help determine that),
this patch will likely need to wait until general development
reopens sometime in May.  In any case, it's a maintainer's call
to approve it.

Thanks again!

Martin

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

[David Lamparter]

On Wed, Mar 10, 2021 at 10:01:39AM -0700, Martin Sebor wrote:
> On 3/9/21 6:46 PM, David Lamparter wrote:
> > On Wed, Mar 10, 2021 at 02:28:15AM +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.

> A few comments on the patch.  It took me a bit at first to see
> the problem it tries to solve.  I see the improvement but I'm not
> sure it's quite correct, or what in the test verifies the desired
> outcome.

I've tried making it a bit more clear in a bug opened as requested:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99526

> The warning on line 12 before the change is:
> 
> cast-5.c:13:7: warning: conversion from ‘int’ to ‘unsigned char’ may 
> change value [-Wconversion]
> 
> and after:
> 
> cast-5.c:13:7: warning: conversion from ‘qualtypedefname’ {aka ‘int’} to 
> ‘unsigned char’ may change value [-Wconversion]
> 
> There's no volatile, so the test passes either way.  More important,
> though, qualtypedefname is a typedef for volatile int but the AKA
> type says it's int.  So the new output doesn't seem right.

The new output is actually correct since casts produce rvalues.  I was
trying to put together a test for that to make sure the volatile is NOT
there (in an earlier version it was, and that actually broke things.)

That said, I seem to have b0rked the test anyway since on closer look it
says: "ERROR: gcc.dg/cast-5.c: unknown dg option: */ for */"  (I had
missed that since I looked for PASS/FAIL...) - so need to fix that
anyway.

> In any case, the test should fail without the patch.  Using
> dg-warning to look for both the expected typedef name and the AKA
> type would seem like a better to do that than dg-bogus.

Thanks for the input, I need to look a bit more at how this whole stuff
works.  Going to poke it a bit more...

Also, in seeing your response re. the volatile, it occured to me that
stripping qualifiers on a typedef and still calling it the typedef is
unhelpful.  Ideally, my goal is:

typedef int i;
typedef const int ci;
typeof((i)x) = i
typeof((ci)x) = int  (<-- w/ my patch = ci; this is confusing)
typeof((const i)x) = i

(Without my patch, all 3 typeofs come out as plain int.)

So I think I need to somehow check whether the qualifiers are part of
the typedef or not;  essentially peel typedefs until it's one without
qualifiers.

> As an aside, although it's not a requirement, it's helpful to open
> a bug first, before submitting a patch. [...]

Linked above :)

> Finally, GCC is in a regression-fixing stage now, so unless this
> problem is one (having a bug report would also help determine that),
> this patch will likely need to wait until general development
> reopens sometime in May.  In any case, it's a maintainer's call
> to approve it.

Ah, I hadn't seen that.  My bad.


Thanks for the input!

-David

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

[David Lamparter]

On Wed, Mar 10, 2021 at 08:02:16PM +0100, David Lamparter wrote:
> Also, in seeing your response re. the volatile, it occured to me that
> stripping qualifiers on a typedef and still calling it the typedef is
> unhelpful.  Ideally, my goal is:
> 
> typedef int i;
> typedef const int ci;
> typeof((i)x) = i
> typeof((ci)x) = int  (<-- w/ my patch = ci; this is confusing)

Clarification:  with the current patch, the type of (ci)x is *called*
"ci", but doesn't actually have the const qualifier.  That's the
confusing part.  The const (or volatile) is stripped off the typedef in
the added build_qualified_type() call, but that now-different type is
still referred to as "ci"...

> typeof((const i)x) = i

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

[David Lamparter]

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


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-c-don-t-drop-typedef-information-in-casts.patch --]
[-- Type: text/x-patch; name="0001-c-don-t-drop-typedef-information-in-casts.patch", Size: 4050 bytes --]

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  } */
+}

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

[Jakub Jelinek]

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

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

[David Lamparter]

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

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

[David Lamparter]

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


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                    | 39 ++++++++++++++++++++++++++---
 gcc/testsuite/gcc.dg/cast-typedef.c | 35 ++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cast-typedef.c


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-c-don-t-drop-typedef-information-in-casts.patch --]
[-- Type: text/x-patch; name="0001-c-don-t-drop-typedef-information-in-casts.patch", Size: 4406 bytes --]

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 4e6d369c4c9c..c3c0d0c3cf55 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,39 @@ 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;
+
+  for (walk_type = type;
+       walk_type && TYPE_NAME (walk_type)
+	 && TREE_CODE (TYPE_NAME (walk_type)) == TYPE_DECL;
+       walk_type = DECL_ORIGINAL_TYPE (TYPE_NAME (walk_type)))
+    {
+      tree walk_unqual, orig_type, orig_decl;
+
+      walk_unqual = build_qualified_type (walk_type, TYPE_UNQUALIFIED);
+
+      orig_decl = lookup_name (TYPE_IDENTIFIER (walk_type));
+      if (!orig_decl || TREE_CODE (orig_decl) != TYPE_DECL)
+	continue;
+      orig_type = TREE_TYPE (orig_decl);
+
+      if (walk_unqual == orig_type)
+	{
+	  res_type = walk_unqual;
+	  break;
+	}
+    }
+
   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 +5897,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 +6051,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 +6087,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..3058e5a0b190
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cast-typedef.c
@@ -0,0 +1,35 @@
+/* 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  } */
+
+  /* shadow "typedefname" & make sure it's not used */
+  typedef short typedefname;
+  f2 ((qual2) val); /* { dg-warning "int" } */
+
+  /* { dg-bogus "typedefname" "retaining a shadowed typedef would be confusing" { target { "*-*-*" } } 32 } */
+}

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

[David Lamparter]

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


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                    | 39 ++++++++++++++++++++++++++---
 gcc/testsuite/gcc.dg/cast-typedef.c | 35 ++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cast-typedef.c
---

Hi GCC hackers,


now that gcc 12 is open for development, I'd like to submit this patch
for reconsideration.  I've already gone through a bit of feedback while
the gcc 11 release was happening, cf. here:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566513.html

I've repeated my testing (full bootstrap & make check on x86_64) and
found nothing changed.


Cheers,

-David


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-c-don-t-drop-typedef-information-in-casts.patch --]
[-- Type: text/x-patch; name="0001-c-don-t-drop-typedef-information-in-casts.patch", Size: 4406 bytes --]

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index fdc7bb6125c2..ba6014726a4b 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -5876,6 +5876,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);
@@ -5896,7 +5897,39 @@ 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;
+
+  for (walk_type = type;
+       walk_type && TYPE_NAME (walk_type)
+	 && TREE_CODE (TYPE_NAME (walk_type)) == TYPE_DECL;
+       walk_type = DECL_ORIGINAL_TYPE (TYPE_NAME (walk_type)))
+    {
+      tree walk_unqual, orig_type, orig_decl;
+
+      walk_unqual = build_qualified_type (walk_type, TYPE_UNQUALIFIED);
+
+      orig_decl = lookup_name (TYPE_IDENTIFIER (walk_type));
+      if (!orig_decl || TREE_CODE (orig_decl) != TYPE_DECL)
+	continue;
+      orig_type = TREE_TYPE (orig_decl);
+
+      if (walk_unqual == orig_type)
+	{
+	  res_type = walk_unqual;
+	  break;
+	}
+    }
+
   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)
     {
@@ -5924,7 +5957,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)
     {
@@ -6078,7 +6111,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))
@@ -6114,7 +6147,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..3058e5a0b190
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cast-typedef.c
@@ -0,0 +1,35 @@
+/* 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  } */
+
+  /* shadow "typedefname" & make sure it's not used */
+  typedef short typedefname;
+  f2 ((qual2) val); /* { dg-warning "int" } */
+
+  /* { dg-bogus "typedefname" "retaining a shadowed typedef would be confusing" { target { "*-*-*" } } 32 } */
+}

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

[David Lamparter]

On Fri, May 07, 2021 at 06:09:35PM +0200, 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.

Any comments on this?  Anything I can do to move this forward?  Or is it
not suitable to be picked up?

Cheers,


-David

> [...]
>
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index fdc7bb6125c2..ba6014726a4b 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -5876,6 +5876,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);
> @@ -5896,7 +5897,39 @@ 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;
> +
> +  for (walk_type = type;
> +       walk_type && TYPE_NAME (walk_type)
> +	 && TREE_CODE (TYPE_NAME (walk_type)) == TYPE_DECL;
> +       walk_type = DECL_ORIGINAL_TYPE (TYPE_NAME (walk_type)))
> +    {
> +      tree walk_unqual, orig_type, orig_decl;
> +
> +      walk_unqual = build_qualified_type (walk_type, TYPE_UNQUALIFIED);
> +
> +      orig_decl = lookup_name (TYPE_IDENTIFIER (walk_type));
> +      if (!orig_decl || TREE_CODE (orig_decl) != TYPE_DECL)
> +	continue;
> +      orig_type = TREE_TYPE (orig_decl);
> +
> +      if (walk_unqual == orig_type)
> +	{
> +	  res_type = walk_unqual;
> +	  break;
> +	}
> +    }
> +
>    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)
>      {
> @@ -5924,7 +5957,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)
>      {
> @@ -6078,7 +6111,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))
> @@ -6114,7 +6147,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..3058e5a0b190
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/cast-typedef.c
> @@ -0,0 +1,35 @@
> +/* 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  } */
> +
> +  /* shadow "typedefname" & make sure it's not used */
> +  typedef short typedefname;
> +  f2 ((qual2) val); /* { dg-warning "int" } */
> +
> +  /* { dg-bogus "typedefname" "retaining a shadowed typedef would be confusing" { target { "*-*-*" } } 32 } */
> +}

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

[Joseph Myers]

On Tue, 18 May 2021, David Lamparter wrote:

> On Fri, May 07, 2021 at 06:09:35PM +0200, 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.
> 
> Any comments on this?  Anything I can do to move this forward?  Or is it
> not suitable to be picked up?

Every time I've looked at it, I've been uneasy about the same issue 
previously mentioned in review of how the patch looks up names while 
processing a cast, which seems very suspicious to me as it did to previous 
reviewers - all name lookup should have been done earlier in parsing, not 
in the subsequent semantic analysis involved in processing or diagnosing a 
cast.  If you think there is some reason that late name lookup is either 
necessary or correct, it would help to have a rather longer commit message 
that explains the reasoning involved at length (patch submissions should 
always be self-contained, so include a self-contained commit message 
explaining whatever issues may arise in review of the patch, including any 
issues from previous reviews that haven't been addressed through changes 
to the patch).

Because the name lookup is suspicious, it probably *also* needs 
explanation in a comment in the code for why it wasn't done earlier in 
parsing.

-- 
Joseph S. Myers
joseph@codesourcery.com