c: don't drop typedef information in casts

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

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

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

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

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