Bug 34389 - -Wconversion produces wrong warning
Summary: -Wconversion produces wrong warning
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 35701
  Show dependency treegraph
 
Reported: 2007-12-08 04:15 UTC by merkert
Modified: 2018-02-20 10:45 UTC (History)
4 users (show)

See Also:
Host:
Target: gcc (GCC) 4.3.0 20071130 (experimental)
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-02-15 18:04:44


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description merkert 2007-12-08 04:15:18 UTC
If compiled with gcc -c -Wconversion the following code produces the warning:
test.c:7: warning: conversion to ‘short int’ from ‘int’ may alter its value

but if -DNO_WARNING is turned on it doesn't even though it should be same code.

cat > test.c <<EOF
short  mask(short x)
{
#if NO_WARNING
  short y = 0x7fff;
  return x&y;
#else
  return x & (short)0x7fff;
#endif
}
EOF


On a side node, I think it would be better to also change the warning to :
"conversion from 'int' to 'short int' may alter its value", which i find easier  to read.
Comment 1 Manuel López-Ibáñez 2007-12-13 21:38:26 UTC
Confirmed. 

When the expression "x & (short)0x7fff" reaches the warning code, it sees "(int)x & 0x7fff". So it rightfully warns that an "int" expression is converted to "short". This comes from conversions performed in build_binary_op. Not sure what the appropriate fix would be...
Comment 2 Andrew Pinski 2007-12-13 21:53:17 UTC
I think this is related to PR 32643.
Comment 3 Manuel López-Ibáñez 2007-12-17 11:57:14 UTC
(In reply to comment #2)
> I think this is related to PR 32643.
> 

How is it related? There is no overflow here, is there? It is related in the sense that build_binary_op is playing tricks with bitwise AND ?
Comment 4 Manuel López-Ibáñez 2008-01-07 10:40:23 UTC
@Andrew,

I would like to fix this before GCC 4.3 is released but I just don't know how. Any hints?
Comment 5 Manuel López-Ibáñez 2008-01-23 18:00:24 UTC
I don't think this will be fixed before GCC 4.3 is released, which is a shame. But if anyone has a suggestion on how to fix this, I will try to submit a patch ASAP.
Comment 6 Manuel López-Ibáñez 2008-02-15 16:34:08 UTC
Some more findings:

build_binary_op receives two parameters of type int: '(int)x' and '0x7fff'
then it performs the shorten magic that seems right and produces:

<bit_and_expr 
   type <integer_type short int 
    arg 0 <parm_decl x type <integer_type short int>
    arg 1 <integer_cst type <integer_type short int> constant invariant 32767>>

which looks good. But just before returning, it converts that to 'int', what results in:

<bit_and_expr 
    type <integer_type int
  arg 0 <nop_expr type <integer_type int>
        arg 0 <parm_decl x type <integer_type short int>>
    arg 1 <integer_cst type <integer_type int>>

which causes problems later. If instead it returned:

<nop_expr type <integer_type int>
  <bit_and_expr 
     type <integer_type short int 
      arg 0 <parm_decl x type <integer_type short int>
      arg 1 <integer_cst type <integer_type short int> constant invariant>>

No warning would be generated!

Why bother with the shorten magic if convert is going to override it? Where it is overriding it?

The current code converts the constant back and forth between short int and int at least 3 times.

Any ideas on how to stop this madness?
Comment 7 Manuel López-Ibáñez 2008-02-15 16:53:20 UTC
So fold_unary is transforming (T)(x & c) into (T)x & (T)c. Which is exactly the opposite transformation that build_binary_op just performed! Weird...

So I see two options:

* Either teach fold_unary to avoid this particular transformation in this particular case.

* Hack conversion_warning to realize that (short)((int)x & c) is the same as (short)(x & c) if x is signed int and c fits in a signed int. Which will be undoing the transformation once more (fourth? fifth time?). And I am not even sure how to do this.


Comment 8 Manuel López-Ibáñez 2008-02-15 18:04:44 UTC
This patch fixes this bug but it is an ugly hack. Perhaps such an ugly hack is the only thing we can do at the moment.

Once bootstrapping + testing finishes, I will submit.

Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c      (revision 132310)
+++ gcc/c-common.c      (working copy)
@@ -1270,65 +1270,98 @@ conversion_warning (tree type, tree expr
                  "conversion to %qT alters %qT constant value",
                  type, TREE_TYPE (expr));
     }
   else /* 'expr' is not a constant.  */
     {
+      tree expr_type = TREE_TYPE (expr);
+
       /* Warn for real types converted to integer types.  */
-      if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      if (TREE_CODE (expr_type) == REAL_TYPE
           && TREE_CODE (type) == INTEGER_TYPE)
         give_warning = true;

-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == INTEGER_TYPE)
         {
          /* Don't warn about unsigned char y = 0xff, x = (int) y;  */
          expr = get_unwidened (expr, 0);
+         expr_type = TREE_TYPE (expr);

+         /* Don't warn for short y; short x = ((int)y & 0xff);  */
+         if (TREE_CODE (expr) == BIT_AND_EXPR
+             && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST)
+           {
+             int unsigned0, unsigned1;
+             tree arg0, arg1;
+             tree tmp_type;
+             tree op0, op1;
+
+             op0 = convert (expr_type, TREE_OPERAND (expr, 0));
+             op1 = convert (expr_type, TREE_OPERAND (expr, 1));
+
+             arg0 = get_narrower (op0, &unsigned0);
+             arg1 = get_narrower (op1, &unsigned1);
+
+             /* Handle the case that OP0 (or OP1) does not *contain* a conversion
+                but it *requires* conversion to TYPE.  */
+             if ((TYPE_PRECISION (TREE_TYPE (op0))
+                  == TYPE_PRECISION (TREE_TYPE (arg0)))
+                 && TREE_TYPE (op0) != expr_type)
+               unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+
+             if (TYPE_PRECISION (TREE_TYPE (arg0)) < TYPE_PRECISION (expr_type)
+                 && (tmp_type
+                     = c_common_signed_or_unsigned_type (unsigned0,
+                                                          TREE_TYPE (arg0)))
+                 && !POINTER_TYPE_P (tmp_type)
+                 && int_fits_type_p (arg1, tmp_type))
+               expr_type = tmp_type;
+           }
           /* Warn for integer types converted to smaller integer types.  */
-          if (formal_prec < TYPE_PRECISION (TREE_TYPE (expr)))
+          if (formal_prec < TYPE_PRECISION (expr_type))
            give_warning = true;

          /* When they are the same width but different signedness,
             then the value may change.  */
-         else if ((formal_prec == TYPE_PRECISION (TREE_TYPE (expr))
-                   && TYPE_UNSIGNED (TREE_TYPE (expr)) != TYPE_UNSIGNED (type))
+         else if ((formal_prec == TYPE_PRECISION (expr_type)
+                   && TYPE_UNSIGNED (expr_type) != TYPE_UNSIGNED (type))
                   /* Even when converted to a bigger type, if the type is
                      unsigned but expr is signed, then negative values
                      will be changed.  */
-                  || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (TREE_TYPE (expr))))
+                  || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (expr_type)))
            warning (OPT_Wsign_conversion,
                     "conversion to %qT from %qT may change the sign of the result",
-                    type, TREE_TYPE (expr));
+                    type, expr_type);
         }

       /* Warn for integer types converted to real types if and only if
          all the range of values of the integer type cannot be
          represented by the real type.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == REAL_TYPE)
         {
-          tree type_low_bound = TYPE_MIN_VALUE (TREE_TYPE (expr));
-          tree type_high_bound = TYPE_MAX_VALUE (TREE_TYPE (expr));
+          tree type_low_bound = TYPE_MIN_VALUE (expr_type);
+          tree type_high_bound = TYPE_MAX_VALUE (expr_type);
           REAL_VALUE_TYPE real_low_bound = real_value_from_int_cst (0, type_low_bound);
           REAL_VALUE_TYPE real_high_bound = real_value_from_int_cst (0, type_high_bound);

           if (!exact_real_truncate (TYPE_MODE (type), &real_low_bound)
               || !exact_real_truncate (TYPE_MODE (type), &real_high_bound))
             give_warning = true;
         }

       /* Warn for real types converted to smaller real types.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      else if (TREE_CODE (expr_type) == REAL_TYPE
                && TREE_CODE (type) == REAL_TYPE
-               && formal_prec < TYPE_PRECISION (TREE_TYPE (expr)))
+               && formal_prec < TYPE_PRECISION (expr_type))
         give_warning = true;


       if (give_warning)
         warning (OPT_Wconversion,
                  "conversion to %qT from %qT may alter its value",
-                 type, TREE_TYPE (expr));
+                 type, expr_type);
     }
 }

 /* Produce warnings after a conversion. RESULT is the result of
    converting EXPR to TYPE.  This is a helper function for
Comment 9 rainy6144 2008-04-23 07:20:43 UTC
Does the patch also fix the warning for conditional expressions?  For example:

short f(int cond, short x, short y)
{
  return cond ? x : y;
}
Comment 10 Manuel López-Ibáñez 2008-06-08 17:30:19 UTC
(In reply to comment #9)
> Does the patch also fix the warning for conditional expressions?  For example:
> 
> short f(int cond, short x, short y)
> {
>   return cond ? x : y;
> }
> 

No, that is a completely different issue.
Comment 11 Manuel López-Ibáñez 2008-07-30 08:31:56 UTC
Subject: Bug 34389

Author: manu
Date: Wed Jul 30 08:30:32 2008
New Revision: 138296

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138296
Log:
2008-07-30  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

	PR 34389
	* c-typeck.c (build_binary_op): Encapsulate code into...
	* c-common.c (shorten_binary_op): ...this new function.
	(conversion_warning): Use the new function. Handle non-negative
	constant in bitwise-and.
	* c-common.h (shorten_binary_op): Declare.
cp/
	* typeck.c (build_binary_op): Encapsulate code into
	shorten_binary_op.
testsuite/
	* gcc.dg/Wconversion-pr34389.c: New.
	* g++.dg/warn/Wconversion-pr34389.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C
    trunk/gcc/testsuite/gcc.dg/Wconversion-pr34389.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-common.c
    trunk/gcc/c-common.h
    trunk/gcc/c-typeck.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/typeck.c
    trunk/gcc/testsuite/ChangeLog

Comment 12 Manuel López-Ibáñez 2008-07-30 08:36:52 UTC
This should be mostly fixed except the following testcase in C++:

short  mask1(short x)
{
  short y = 0x7fff;
  return x & y; /* { dg-bogus "conversion" "conversion" { xfail *-*-* } 8 } */
}

This works in C, so it seems "same_type" does different things in C and in C++. I don't have time to investigate this at the moment so any help is appreciated.
Comment 13 Manuel López-Ibáñez 2008-08-01 19:36:24 UTC
I created PR 37004 to cover the issue with C++ front-end. Apart from that, this bug is FIXED in GCC 4.4.