Bug 53001

Summary: -Wfloat-conversion should be available to warn about floating point errors
Product: gcc Reporter: Joshua Cogliati <jjcogliati-r1>
Component: cAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: enhancement CC: jjcogliati-r1, manu
Priority: P3    
Version: unknown   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed:
Attachments: Patch to add -Wfloat-conversion option
some floating conversion issues
Patch to add -Wfloat-conversion option
Patch to add -Wfloat-conversion option
Patch to add -Wfloat-conversion option against trunk
Patch to add -Wfloat-conversion option against trunk
Patch to add -Wfloat-conversion option against trunk
Patch to add -Wfloat-conversion option against trunk
Patch to fixup gcc float conversions in GCC
Patch to add -Wfloat-conversion option against trunk with testcases
Patch to add -Wfloat-conversion option against trunk with testcases
Patch to add -Wfloat-conversion option against trunk with new testcase
Patch to add -Wfloat-conversion option against trunk with new testcase and detailed warnings

Description Joshua Cogliati 2012-04-16 03:15:44 UTC
This is a request to add a new warning that warns on the subset of -Wconversion warnings that involve floating point numbers.  For example, with -Wfloat-conversion this would cause a warning:

int main(int argc, char ** argv) {
  int i = 3.14;
  return i;
}

I think this could mostly be done by modifying gcc/c-family/c-common.c unsafe_conversion_p to add the ability to only warn on conversions where REAL_TYPE or REAL_CST are involved.  

The reason -Wconversion is not sufficient is that using it can cause hundreds of warnings, almost all of which are integer size warnings (many of which are in libraries (such as libMesh)).  I would like to be able to enable something like -Wfloat-conversion that would catch errors involving floating point number conversions, while ignoring the integer size conversions.
Comment 1 Manuel López-Ibáñez 2012-04-16 11:41:24 UTC
(In reply to comment #0)
> This is a request to add a new warning that warns on the subset of -Wconversion
> warnings that involve floating point numbers.  For example, with
> -Wfloat-conversion this would cause a warning:

Should it also warn for non-literals?

int foo(double x)
{
   return x;
}

> I think this could mostly be done by modifying gcc/c-family/c-common.c
> unsafe_conversion_p to add the ability to only warn on conversions where
> REAL_TYPE or REAL_CST are involved.  

Yes, I think it should be easy to implement. You will also need to add a new option to gcc/c.opt and enable -Wfloat-conversion with -Wconversion (grep for OPT_Wimplict and how it handles its suboptions).

Unfortunately, I don't have time to work on this, and probably nobody else has, so you could try to submit a patch: http://gcc.gnu.org/contribute.html
Comment 2 Joshua Cogliati 2012-04-16 12:16:45 UTC
Yes, it should also warn for non-constants, and also for other floating decreases in accuracy such as: 

float foo(double x) {
  return x;
}

I should have time to create a patch for this before 4.8 goes into stage 3.  Do you think it needs a copyright assignment and if so what paperwork would you need from my employer?
Comment 3 Manuel López-Ibáñez 2012-04-16 12:36:59 UTC
(In reply to comment #2)
> I should have time to create a patch for this before 4.8 goes into stage 3.  Do
> you think it needs a copyright assignment and if so what paperwork would you
> need from my employer?

I am afraid so. See: https://www.gnu.org/prep/maintain/maintain.html#Legal-Matters

Write to gcc@gcc.gnu.org asking for the documents. There are several people there dealing with this matter. Also, let us know if you find any problems.
Comment 4 Manuel López-Ibáñez 2012-06-07 14:03:45 UTC
*** Bug 53603 has been marked as a duplicate of this bug. ***
Comment 5 Joshua Cogliati 2012-06-22 12:29:14 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > I should have time to create a patch for this before 4.8 goes into stage 3.  Do
> > you think it needs a copyright assignment and if so what paperwork would you
> > need from my employer?
> 
> I am afraid so. See:
> https://www.gnu.org/prep/maintain/maintain.html#Legal-Matters
> 
> Write to gcc@gcc.gnu.org asking for the documents. There are several people
> there dealing with this matter. Also, let us know if you find any problems.

I have written to to gcc@gcc.gnu.org, and submitted the Employer disclaimer to my employer, but my employer is reluctant to sign it. I may not be able to create the patch because of that.
Comment 6 Manuel López-Ibáñez 2012-06-22 12:41:05 UTC
(In reply to comment #5)
> 
> I have written to to gcc@gcc.gnu.org, and submitted the Employer disclaimer to
> my employer, but my employer is reluctant to sign it. I may not be able to
> create the patch because of that.

Write to both gcc@gcc.gnu.org and assign@gnu.org to explain your case. They may be able to work out an arrangement. Sadly, this situation is fairly common.
Comment 7 Joshua Cogliati 2013-09-02 19:50:51 UTC
I wrote the patch for 4.8.1 (which is the easy part), now I will see if I can get approval to submit it through the bureaucracies (the hard part).
Comment 8 Joshua Cogliati 2013-09-20 16:27:15 UTC
Created attachment 30870 [details]
Patch to add -Wfloat-conversion option

This patch add a -Wfloat-conversion option.  It adds an enumeration type called conversion_safety, with a SAFE_CONVERSION being 0, and unsafe ones being non-zero.  It changes the unsafe_conversion_p to return this enumeration.  This allows the floating point conversions to be handled separately.  

Someone who understands the c.opt file better than I should check that I did it correctly. 

-Wconversion implies -Wfloat-conversion.   -Wall does not, but it might possibly be worth considering.
Comment 9 Joshua Cogliati 2013-09-20 16:30:28 UTC
Created attachment 30871 [details]
some floating conversion issues

This tests three of the floating conversion reduction in precision issues.  

P.S. Copyright assignment paperwork from both myself and my employer for GCC has been sent to copyright-clerk
Comment 10 Richard Neill 2013-09-20 16:39:46 UTC
Thank you for adding this - I will certainly find it useful!

May I suggest that this warning be implicitly enabled by -Wextra, (even if it's not enabled by -Wall) ?
Comment 11 Joshua Cogliati 2013-09-20 17:27:11 UTC
Created attachment 30873 [details]
Patch to add -Wfloat-conversion option

This version also is enabled with -Wextra.  

$ gcc -o float_test float_test.c
$ gcc -Wfloat-conversion -o float_test float_test.c
float_test.c: In function ‘main’:
float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion]
   int i = 3.14;
   ^
float_test.c: In function ‘foo’:
float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
float_test.c: In function ‘foo2’:
float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
$ gcc -Wconversion -o float_test float_test.c
float_test.c: In function ‘main’:
float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion]
   int i = 3.14;
   ^
float_test.c: In function ‘foo’:
float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
float_test.c: In function ‘foo2’:
float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
$ gcc -Wextra -o float_test float_test.c
float_test.c: In function ‘main’:
float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion]
   int i = 3.14;
   ^
float_test.c: In function ‘foo’:
float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
float_test.c: In function ‘foo2’:
float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
Comment 12 Manuel López-Ibáñez 2013-09-20 20:27:06 UTC
The patch looks mostly correct to me, but you will need a few extra details before a maintainer (not me!) can accept it. 

* You need to bootstrap + run the testsuite. You don't really need to add testcases, because there are quite a few already in the testsuite for Wconversion, but perhaps to be sure that your patch is correct, you could check those and change -Wconversion for -Wfloat-conversion where it should trigger.

* You need to write a Changelog and submit to gcc-patches@gcc.gnu.org. When submitting CC dodji@redhat.com who is the diagnostics maintainer.

* There are some potential issues in the patch that maintainers may complain 

diff -ur gcc-4.8.1_orig/gcc/c-family/c-common.c gcc-4.8.1/gcc/c-family/c-common.c
--- gcc-4.8.1_orig/gcc/c-family/c-common.c	2013-05-14 14:52:27.000000000 -0600
+++ gcc-4.8.1/gcc/c-family/c-common.c	2013-09-02 13:14:01.000000000 -0600
@@ -294,6 +294,8 @@
   {NULL, 0, 0},
 };
 
+enum conversion_safety {SAFE_CONVERSION=0,UNSAFE_OTHER,UNSAFE_SIGN,UNSAFE_REAL};
+

Perhaps you should add a bit of documentation of what each value means? Also, you don't seem to use UNSAFE_SIGN.
Also, some extra whitespace around =, after ',' and {,  and before }.

-extern bool unsafe_conversion_p (tree, tree, bool);

This function is used in the C++ FE, you cannot make it static. Did you bootstrap the compiler with the patch?

+      if ((conversion_kind = unsafe_conversion_p (type, expr, true)))

I don't think this is going to be accepted. You should split the assignment and the test.

+Wfloat-conversion
+C ObjC C++ ObjC++ Var(warn_float_conversion) LangEnabledBy(C ObjC C++,Wconversion) EnabledBy(Wextra)

This looks ok to me, but you could add ObjC++ in the LangEnabledBy part.

+This includes conversions from real to integer, and from higher precision
+real to lower precision real values.  This option is also enabled by 
+@option{-Wconversion}.

and by @option{-Wextra}! You should also add it to the list of things enabled by -Wextra (look where -Wextra is defined).
Comment 13 Manuel López-Ibáñez 2013-09-20 20:33:05 UTC
Another potential issue is that your patch is against 4.8.1, but this is not a regression, so it is very likely that maintainers will ask you to submit a patch with respect to current trunk: http://www.gnu.org/software/gcc/svn.html
Comment 14 Joshua Cogliati 2013-09-21 00:25:09 UTC
Manuel López-Ibáñez, thank you for the patch review.  I will make the changes you requested, and rebase it against svn trunk, and run bootstrap and testsuite.  It may be a few weeks before I get this all done (depending on available time).

UNSAFE_SIGN is unused, but when I was doing the patch, I noticed that unsafe_conversion_p basically does not report sign conversion errors, instead it does the warning directly inside the function.  So there definitely is a distinct unsafe conversion type, but I am not sure if I should remove the constant entirely or not.  Either way I will try and document the behavior better.
Comment 15 Joshua Cogliati 2013-09-22 23:02:48 UTC
Created attachment 30882 [details]
Patch to add -Wfloat-conversion option

Adding documentation, formating better, removing assignment from test.  Still based on 4.8.1 (which will be fixed later)

Still a static function, C++ seems to be bootstraped fine.
Comment 16 Joshua Cogliati 2013-09-26 03:32:46 UTC
Created attachment 30899 [details]
Patch to add -Wfloat-conversion option against trunk

This version is against gcc trunk (rev 202818).  For trunk, I had to unmake unsafe_conversion_p being static (it is now extern again.)  

This does not bootstrap trunk yet, because gcc has floating conversion issues and with this being enabled by -Wextra and with -Werror, gcc fails to build.
Comment 17 Manuel López-Ibáñez 2013-09-26 11:50:13 UTC
(In reply to Joshua Cogliati from comment #16)
> This does not bootstrap trunk yet, because gcc has floating conversion
> issues and with this being enabled by -Wextra and with -Werror, gcc fails to
> build.

Are those real bugs or normal code?
Comment 18 Joshua Cogliati 2013-09-27 02:26:29 UTC
(In reply to Manuel López-Ibáñez from comment #17)
> (In reply to Joshua Cogliati from comment #16)
> > This does not bootstrap trunk yet, because gcc has floating conversion
> > issues and with this being enabled by -Wextra and with -Werror, gcc fails to
> > build.
> 
> Are those real bugs or normal code?

So far they seem to be normal code.  I'll try eliminating the warning on a few, but if there are too many I'll just split the patch into the basic warning and one that occurs on -Wextra.
Comment 19 Joshua Cogliati 2013-09-27 12:06:19 UTC
Created attachment 30913 [details]
Patch to add -Wfloat-conversion option against trunk

This version is against gcc trunk (rev 202818).  It now bootstraps.  

It adds about ten casts so that the existing float conversions in gcc are now explicit instead of implicit so that gcc can bootstrap even with the new warning.
Comment 20 Joshua Cogliati 2013-09-30 12:13:48 UTC
Created attachment 30937 [details]
Patch to add -Wfloat-conversion option against trunk

Added one more changed needed to get it to compile (which only occured if I did an rm -Rf in the build directry, the make bootstrap didn't catch the libcpp warning found.)  

The only thing I have left is to possibly modify the test scripts (to use -Wfloat-conversion instead of -Wconversion for the relevant tests) and recompile against the current trunk.
Comment 21 Manuel López-Ibáñez 2013-10-02 08:29:28 UTC
(In reply to Joshua Cogliati from comment #20)
> Created attachment 30937 [details]
> Patch to add -Wfloat-conversion option against trunk
> 
> Added one more changed needed to get it to compile (which only occured if I
> did an rm -Rf in the build directry, the make bootstrap didn't catch the
> libcpp warning found.)  
> 
> The only thing I have left is to possibly modify the test scripts (to use
> -Wfloat-conversion instead of -Wconversion for the relevant tests) and
> recompile against the current trunk.

I would humbly suggest that you first recompile against current trunk, then submit to gcc-patches and CC Dodji and Jason Merrill (you can find the emails in MAINTAINERS) asking for comments before spending time and effort on updating the test cases. The maintainers may ask you to change the name, or they might not like that it triggers so often in GCC (so they may ask you to not enable it with -Wextra), or they might simply not like the patch at all. 

Please mention that it is your first patch to GCC and your copyright assignment is already in order. People are reluctant to review patches unless the papers are in order since there is a high chance that they will never be committed.
Comment 22 Joshua Cogliati 2013-10-11 02:23:46 UTC
Created attachment 30979 [details]
Patch to add -Wfloat-conversion option against trunk

This version only does the -Wfloat-conversion.  It does not do the -Wextra or the changes needed to get -Wextra to bootstrap.  It starts to change the test cases.  This work is to satisfy the request on the gcc-patch list.
Comment 23 Joshua Cogliati 2013-10-11 02:31:53 UTC
Created attachment 30980 [details]
Patch to fixup gcc float conversions in GCC

This changes the float conversions in gcc so that gcc bootstraps if float conversions are warned about with -Wextra.  Splitting the patch was requested on gcc-patches.


(Note to self: patches were created with 
svn diff --diff-cmd diff -x -up
)
Comment 24 Joshua Cogliati 2013-10-13 13:48:39 UTC
Created attachment 30994 [details]
Patch to add -Wfloat-conversion option against trunk with testcases

This version modifies a few more testcases.
Comment 25 Joshua Cogliati 2013-10-16 02:47:55 UTC
Created attachment 31014 [details]
Patch to add -Wfloat-conversion option against trunk with testcases

Changes some formatting and comments as requested on mailing list.
Comment 26 Joshua Cogliati 2013-10-22 01:37:07 UTC
Created attachment 31065 [details]
Patch to add -Wfloat-conversion option against trunk with new testcase

This adds a new testcase instead of using existing testcases.
Comment 27 Joshua Cogliati 2013-10-27 13:16:58 UTC
Created attachment 31097 [details]
Patch to add -Wfloat-conversion option against trunk with new testcase and detailed warnings

This uses a detailed warnings in the testcase.  I am not sure if this is a good idea or not since it might get invalidated if the types change.  It bootstraps and the new testcase works on x86_64-unknown-linux-gnu
Comment 28 Manuel López-Ibáñez 2013-11-20 07:15:43 UTC
Author: manu
Date: Wed Nov 20 07:15:40 2013
New Revision: 205090

URL: http://gcc.gnu.org/viewcvs?rev=205090&root=gcc&view=rev
Log:
2013-11-19  Joshua J Cogliati  <jrincayc@yahoo.com>

	PR c/53001
	Splitting out a -Wfloat-conversion from -Wconversion for
	conversions that lower floating point number precision
	or conversion from floating point numbers to integers.

gcc/c-family/

	* c-common.c (unsafe_conversion_p): Make this function
	return an enumeration with more detail.
	(conversion_warning): Use the new return type of
	unsafe_conversion_p to separately warn either about conversions
	that lower floating point number precision or about the other
	kinds of conversions.
 	* c-common.h (enum conversion_safety): New enumeration.
 	(unsafe_conversion_p): switching return type to
 	conversion_safety enumeration.
	* c.opt: Adding new warning -Wfloat-conversion and
	enabling it with -Wconversion.

gcc/

	* doc/invoke.texi: Adding documentation about
	-Wfloat-conversion.

gcc/testsuite/

	* c-c++-common/Wfloat-conversion.c: Copies relevant
	tests from c-c++-common/Wconversion-real.c,
	gcc.dg/Wconversion-real-integer.c and gcc.dg/pr35635.c into
	new testcase for conversions that are warned about by
	-Wfloat-conversion.

Added:
    trunk/gcc/testsuite/c-c++-common/Wfloat-conversion.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/c-family/c.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
Comment 29 Manuel López-Ibáñez 2013-11-20 07:42:44 UTC
I guess this is fixed for GCC 4.9.
Comment 30 Joshua Cogliati 2013-11-22 13:10:36 UTC
Yes, it is fixed so far as I am concerned.  I checked out gcc trunk 205109, and bootstraped it and tried it out on:

int main(int argc, char ** argv) {
  int i = 3.14;
  return i;
}

int foo(double x)
{
  return x;
}

float foo2(double x) {
  return x;
}

and without the flag it didn't warn, but with the flag it did:
gcc -Wall -Wfloat-conversion -o float_test float_test.c
float_test.c: In function ‘main’:
float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion]
   int i = 3.14;
   ^
float_test.c: In function ‘foo’:
float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^
float_test.c: In function ‘foo2’:
float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion]
   return x;
   ^

I think there are two things remaining to be done.  

1. http://gcc.gnu.org/gcc-4.9/changes.html does not yet list it as a change, 

2. for gcc 4.10 (or what ever the next gcc version is) I think it might be worth considering as a -Wextra or -Wall.  That however would be a separate bug number and would require something like the  Patch to fixup gcc float conversions in GCC and another patch to turn it on with the flag.