public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] c: don't drop typedef information in casts
@ 2021-03-10  1:28 David Lamparter
  2021-03-10  1:46 ` David Lamparter
  0 siblings, 1 reply; 9+ messages in thread
From: David Lamparter @ 2021-03-10  1:28 UTC (permalink / raw)
  To: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] c: don't drop typedef information in casts
  2021-03-10  1:28 [PATCH] c: don't drop typedef information in casts David Lamparter
@ 2021-03-10  1:46 ` David Lamparter
  2021-03-10 17:01   ` Martin Sebor
  0 siblings, 1 reply; 9+ messages in thread
From: David Lamparter @ 2021-03-10  1:46 UTC (permalink / raw)
  To: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] c: don't drop typedef information in casts
  2021-03-10  1:46 ` David Lamparter
@ 2021-03-10 17:01   ` Martin Sebor
  2021-03-10 19:02     ` David Lamparter
  0 siblings, 1 reply; 9+ messages in thread
From: Martin Sebor @ 2021-03-10 17:01 UTC (permalink / raw)
  To: David Lamparter, gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] c: don't drop typedef information in casts
  2021-03-10 17:01   ` Martin Sebor
@ 2021-03-10 19:02     ` David Lamparter
  2021-03-10 19:06       ` David Lamparter
  2021-03-12  3:08       ` [PATCH v2] " David Lamparter
  0 siblings, 2 replies; 9+ messages in thread
From: David Lamparter @ 2021-03-10 19:02 UTC (permalink / raw)
  To: Martin Sebor; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] c: don't drop typedef information in casts
  2021-03-10 19:02     ` David Lamparter
@ 2021-03-10 19:06       ` David Lamparter
  2021-03-12  3:08       ` [PATCH v2] " David Lamparter
  1 sibling, 0 replies; 9+ messages in thread
From: David Lamparter @ 2021-03-10 19:06 UTC (permalink / raw)
  To: Martin Sebor; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] c: don't drop typedef information in casts
  2021-03-10 19:02     ` David Lamparter
  2021-03-10 19:06       ` David Lamparter
@ 2021-03-12  3:08       ` David Lamparter
  2021-03-12  8:35         ` Jakub Jelinek
  1 sibling, 1 reply; 9+ messages in thread
From: David Lamparter @ 2021-03-12  3:08 UTC (permalink / raw)
  To: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] c: don't drop typedef information in casts
  2021-03-12  3:08       ` [PATCH v2] " David Lamparter
@ 2021-03-12  8:35         ` Jakub Jelinek
  2021-03-15 18:14           ` David Lamparter
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2021-03-12  8:35 UTC (permalink / raw)
  To: David Lamparter; +Cc: gcc-patches

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] c: don't drop typedef information in casts
  2021-03-12  8:35         ` Jakub Jelinek
@ 2021-03-15 18:14           ` David Lamparter
  2021-03-15 18:16             ` [PATCH v3] " David Lamparter
  0 siblings, 1 reply; 9+ messages in thread
From: David Lamparter @ 2021-03-15 18:14 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] c: don't drop typedef information in casts
  2021-03-15 18:14           ` David Lamparter
@ 2021-03-15 18:16             ` David Lamparter
  0 siblings, 0 replies; 9+ messages in thread
From: David Lamparter @ 2021-03-15 18:16 UTC (permalink / raw)
  To: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-15 18:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10  1:28 [PATCH] c: don't drop typedef information in casts David Lamparter
2021-03-10  1:46 ` David Lamparter
2021-03-10 17:01   ` Martin Sebor
2021-03-10 19:02     ` David Lamparter
2021-03-10 19:06       ` David Lamparter
2021-03-12  3:08       ` [PATCH v2] " David Lamparter
2021-03-12  8:35         ` Jakub Jelinek
2021-03-15 18:14           ` David Lamparter
2021-03-15 18:16             ` [PATCH v3] " David Lamparter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).