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>

	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

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

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.