(0010193)
Luis Ibanez (manager)
2008-01-18 13:24

We will enable this in Code/IO and Utilities on Friday Jan 18, and then clean the warnings.

(0010198)
Luis Ibanez (manager)
2008-01-18 14:07

We actually enabled only in Code/IO first since it has to be done on a TARGET by TARGET basis. (Friday Jan 18 2008)

(0010199)
Sean McBride (developer)
2008-01-18 17:13

Sweet! We will keep an eye on our dashboards. I expect with even just Code/IO there will be hundreds of warnings, but that's ok. :)

In my experience we can break down the warnings this emits into a few categories:

1) double -> float conversions:

1a) code like "float x = 1.0". The fix here is either to change to double or to use "1.0f", note the "f".

1b) mismatch with float/double APIs. For example: sin() vs sinf(). The former takes double, the latter float.

2) size_t -> int conversions. In 32 bit these are the same size, in 64 bit size_t is bigger. This applies to code like "int length = sizeof(foo)". We should try to always use size_t or other "size types" like pos_type, off_type, etc.

(0010212)
Sean McBride (developer)
2008-01-22 10:29

Luis, would you like us to fix a few of these? We could fix those of type 1a since the change is very safe. But those of type 2 usually require a better understanding of the whole ITK system as a whole.

Also, the warning count on the dashboards is deceptively high, don't be discouraged. :) Since most of our dashboards build as ppc32, ppc64, intel32 and intel64, the warning count is roughly quadrupled.

(0010224)
Sean McBride (developer)
2008-01-23 10:30

I have fixed a few simple ones, of type 1a, by adding the 'f' as needed.

/cvsroot/Insight/Insight/Code/IO/itkAnalyzeImageIO.cxx,v <-- itkAnalyzeImageIO.cxx
new revision: 1.84; previous revision: 1.83
/cvsroot/Insight/Insight/Code/IO/itkBrains2MaskImageIO.cxx,v <-- itkBrains2MaskImageIO.cxx
new revision: 1.28; previous revision: 1.27
/cvsroot/Insight/Insight/Code/IO/itkGE5ImageIO.cxx,v <-- itkGE5ImageIO.cxx
new revision: 1.35; previous revision: 1.34
/cvsroot/Insight/Insight/Code/IO/itkGEAdwImageIO.cxx,v <-- itkGEAdwImageIO.cxx
new revision: 1.28; previous revision: 1.27
/cvsroot/Insight/Insight/Code/IO/itkIPLCommonImageIO.cxx,v <-- itkIPLCommonImageIO.cxx
new revision: 1.49; previous revision: 1.48
/cvsroot/Insight/Insight/Code/IO/itkSiemensVisionImageIO.cxx,v <-- itkSiemensVisionImageIO.cxx
new revision: 1.16; previous revision: 1.15

(0022960)
Luis Ibanez (manager)
2010-11-07 00:35

We did a large pass at this problem with Brad Lowekamp,
and concluded that it was first, more important to fix the Windows64 bits limitations.

(0023143)
Sean McBride (developer)
2010-11-10 13:06

Is there a related bug for the "Windows64 bits limitations" you speak of?

I imagine that there is overlap, as there is nothing OS-specific in this bug.

Windows uses LLP64 and Unix uses LP64. The only difference is that 'long' is 32bit in the former, but 64 bit in the latter. See:
http://en.wikipedia.org/wiki/LP64#Specific_C-language_data_models [^]

Hopefully Windows has a warning flag that can catch implicit 64 to 32 conversion?

(0023177)
Bradley Lowekamp (developer)
2010-11-11 12:04

The warning for apple proved not to be useful because the majority of the were warning about double to float conversions. While this is good to know it is far from a potential erroneous condition. The real issue of converting from 64-bit integers to 32, was no easily seen through the mess.

(0023196)
Gaetan Lehmann (developer)
2010-11-12 03:35

Sun Studio also has this kind of option -- it is called -xport64. It doesn't seem to warn about double to float conversion though. The following test program

#include <iostream>
int main(int,char**)
{
double d = 1.0;
float f = d;
std::cout << f << std::endl;
size_t s1 = 10;
size_t s2 = 20;
int diff = s2 - s1;
std::cout << diff << std::endl;
int diff2 = static_cast<int>(s2 - s1);
std::cout << diff2 << std::endl;
return 0;
}

produce this output:

[glehmann@itk1 tmp]$ CC -m64 -xport64=implicit toto.c
"toto.c", line 9: Warning: Converting a 64-bit type value to "int" causes truncation.
1 Warning(s) detected.

If it can help, I can submit a nightly build with that option.

(0023206)
Sean McBride (developer)
2010-11-12 12:25

I certainly agree that the double->float warnings are less important (though it would be nice to get to them eventually), and I agree it's a shame that Apple's gcc fork's -Wshorten-64-to-32 doesn't provide finer control.

I tried Gaetan's example, and indeed we get 2 warnings with gcc. I tried with clang, and like Sun Studio, we only get the one for long->int. So when clang can build ITK, I can try turning that warning on...

(0023253)
David Cole (developer)
2010-11-15 07:02

Gaetan, please do submit a nightly build with the -xport64 flag. Let me know which build it is, so I can monitor it after I push some commits.

Thanks,
David

(0023280)
Gaetan Lehmann (developer)
2010-11-16 03:50

I've added the flag to the build SunOS-CC-amd64.
It should appear tomorrow.

(0023334)
David Cole (developer)
2010-11-17 10:12

Hi Gaetan,

Thanks for adding the flags to the sun compiler dashboard submission.

This change in gerrit fixes some of them (which I also observed with the Microsoft 64-bit compiler):
http://review.source.kitware.com/#change,379 [^]

Anybody listening in on this bug, feel free to review the changes and make comments in gerrit.

Thanks.

(0023364)
David Cole (developer)
2010-11-17 18:00

Progress report:

down to 349 failing tests... (down from 619 on last Experimental)
http://www.cdash.org/CDash/buildSummary.php?buildid=777878 [^]

(0023518)
David Cole (developer)
2010-11-23 06:37

Progress report:

down to 4 failing tests (down from 349)
http://www.cdash.org/CDash/buildSummary.php?buildid=784465 [^]

(0024912)
David Cole (developer)
2011-01-19 10:29

Luis, assigning this back to you: would you consider this issue resolved at this point, or are there warnings that still need clean up before we consider it complete?

I can help to reduce/eliminate warnings if there is still work to do... just let me know.

(0024916)
Sean McBride (developer)
2011-01-19 10:47

now that clang can build ITK, I can turn on -Wshorten-64-to-32 on one of my dashboards, it will warn about integer 64 to 32 bit truncation (not floating point).

(0024959)
Sean McBride (developer)
2011-01-20 11:06

So I tried -Wshorten-64-to-32 on my machine, and there are still many many warnings. So many that I stopped the build around half way. The good news is that many of the warnings are from gdcm and vxl and not ITK proper. Also, some warnings are from headers, which inflates the count.

Notes
(0010193) Luis Ibanez (manager) 2008-01-18 13:24	We will enable this in Code/IO and Utilities on Friday Jan 18, and then clean the warnings.

(0010198) Luis Ibanez (manager) 2008-01-18 14:07	We actually enabled only in Code/IO first since it has to be done on a TARGET by TARGET basis. (Friday Jan 18 2008)

(0010199) Sean McBride (developer) 2008-01-18 17:13	Sweet! We will keep an eye on our dashboards. I expect with even just Code/IO there will be hundreds of warnings, but that's ok. :) In my experience we can break down the warnings this emits into a few categories: 1) double -> float conversions: 1a) code like "float x = 1.0". The fix here is either to change to double or to use "1.0f", note the "f". 1b) mismatch with float/double APIs. For example: sin() vs sinf(). The former takes double, the latter float. 2) size_t -> int conversions. In 32 bit these are the same size, in 64 bit size_t is bigger. This applies to code like "int length = sizeof(foo)". We should try to always use size_t or other "size types" like pos_type, off_type, etc.

(0010212) Sean McBride (developer) 2008-01-22 10:29	Luis, would you like us to fix a few of these? We could fix those of type 1a since the change is very safe. But those of type 2 usually require a better understanding of the whole ITK system as a whole. Also, the warning count on the dashboards is deceptively high, don't be discouraged. :) Since most of our dashboards build as ppc32, ppc64, intel32 and intel64, the warning count is roughly quadrupled.

(0010224) Sean McBride (developer) 2008-01-23 10:30	I have fixed a few simple ones, of type 1a, by adding the 'f' as needed. /cvsroot/Insight/Insight/Code/IO/itkAnalyzeImageIO.cxx,v <-- itkAnalyzeImageIO.cxx new revision: 1.84; previous revision: 1.83 /cvsroot/Insight/Insight/Code/IO/itkBrains2MaskImageIO.cxx,v <-- itkBrains2MaskImageIO.cxx new revision: 1.28; previous revision: 1.27 /cvsroot/Insight/Insight/Code/IO/itkGE5ImageIO.cxx,v <-- itkGE5ImageIO.cxx new revision: 1.35; previous revision: 1.34 /cvsroot/Insight/Insight/Code/IO/itkGEAdwImageIO.cxx,v <-- itkGEAdwImageIO.cxx new revision: 1.28; previous revision: 1.27 /cvsroot/Insight/Insight/Code/IO/itkIPLCommonImageIO.cxx,v <-- itkIPLCommonImageIO.cxx new revision: 1.49; previous revision: 1.48 /cvsroot/Insight/Insight/Code/IO/itkSiemensVisionImageIO.cxx,v <-- itkSiemensVisionImageIO.cxx new revision: 1.16; previous revision: 1.15

(0022960) Luis Ibanez (manager) 2010-11-07 00:35	We did a large pass at this problem with Brad Lowekamp, and concluded that it was first, more important to fix the Windows64 bits limitations.

(0023143) Sean McBride (developer) 2010-11-10 13:06	Is there a related bug for the "Windows64 bits limitations" you speak of? I imagine that there is overlap, as there is nothing OS-specific in this bug. Windows uses LLP64 and Unix uses LP64. The only difference is that 'long' is 32bit in the former, but 64 bit in the latter. See: http://en.wikipedia.org/wiki/LP64#Specific_C-language_data_models [^] Hopefully Windows has a warning flag that can catch implicit 64 to 32 conversion?

(0023177) Bradley Lowekamp (developer) 2010-11-11 12:04	The warning for apple proved not to be useful because the majority of the were warning about double to float conversions. While this is good to know it is far from a potential erroneous condition. The real issue of converting from 64-bit integers to 32, was no easily seen through the mess.

(0023196) Gaetan Lehmann (developer) 2010-11-12 03:35	Sun Studio also has this kind of option -- it is called -xport64. It doesn't seem to warn about double to float conversion though. The following test program #include <iostream> int main(int,char**) { double d = 1.0; float f = d; std::cout << f << std::endl; size_t s1 = 10; size_t s2 = 20; int diff = s2 - s1; std::cout << diff << std::endl; int diff2 = static_cast<int>(s2 - s1); std::cout << diff2 << std::endl; return 0; } produce this output: [glehmann@itk1 tmp]$ CC -m64 -xport64=implicit toto.c "toto.c", line 9: Warning: Converting a 64-bit type value to "int" causes truncation. 1 Warning(s) detected. If it can help, I can submit a nightly build with that option.

(0023206) Sean McBride (developer) 2010-11-12 12:25	I certainly agree that the double->float warnings are less important (though it would be nice to get to them eventually), and I agree it's a shame that Apple's gcc fork's -Wshorten-64-to-32 doesn't provide finer control. I tried Gaetan's example, and indeed we get 2 warnings with gcc. I tried with clang, and like Sun Studio, we only get the one for long->int. So when clang can build ITK, I can try turning that warning on...

(0023253) David Cole (developer) 2010-11-15 07:02	Gaetan, please do submit a nightly build with the -xport64 flag. Let me know which build it is, so I can monitor it after I push some commits. Thanks, David

(0023280) Gaetan Lehmann (developer) 2010-11-16 03:50	I've added the flag to the build SunOS-CC-amd64. It should appear tomorrow.

(0023334) David Cole (developer) 2010-11-17 10:12	Hi Gaetan, Thanks for adding the flags to the sun compiler dashboard submission. This change in gerrit fixes some of them (which I also observed with the Microsoft 64-bit compiler): http://review.source.kitware.com/#change,379 [^] Anybody listening in on this bug, feel free to review the changes and make comments in gerrit. Thanks.

(0023364) David Cole (developer) 2010-11-17 18:00	Progress report: down to 349 failing tests... (down from 619 on last Experimental) http://www.cdash.org/CDash/buildSummary.php?buildid=777878 [^]

(0023518) David Cole (developer) 2010-11-23 06:37	Progress report: down to 4 failing tests (down from 349) http://www.cdash.org/CDash/buildSummary.php?buildid=784465 [^]

(0024912) David Cole (developer) 2011-01-19 10:29	Luis, assigning this back to you: would you consider this issue resolved at this point, or are there warnings that still need clean up before we consider it complete? I can help to reduce/eliminate warnings if there is still work to do... just let me know.

(0024916) Sean McBride (developer) 2011-01-19 10:47	now that clang can build ITK, I can turn on -Wshorten-64-to-32 on one of my dashboards, it will warn about integer 64 to 32 bit truncation (not floating point).

(0024959) Sean McBride (developer) 2011-01-20 11:06	So I tried -Wshorten-64-to-32 on my machine, and there are still many many warnings. So many that I stopped the build around half way. The good news is that many of the warnings are from gdcm and vxl and not ITK proper. Also, some warnings are from headers, which inflates the count.

Issue History
Date Modified	Username	Field	Change
2007-12-21 14:38	Sean McBride	New Issue
2007-12-21 14:38	Sean McBride	File Added: ITK64bitwarnings.txt.zip
2008-01-03 15:00	Sean McBride	Description Updated
2008-01-18 13:24	Luis Ibanez	Note Added: 0010193
2008-01-18 14:07	Luis Ibanez	Note Added: 0010198
2008-01-18 17:13	Sean McBride	Note Added: 0010199
2008-01-22 10:29	Sean McBride	Note Added: 0010212
2008-01-23 10:30	Sean McBride	Note Added: 0010224
2008-01-23 16:39	Luis Ibanez	Relationship added	related to 0005529
2008-01-23 16:39	Luis Ibanez	Status	new => assigned
2008-01-23 16:39	Luis Ibanez	Assigned To	=> Luis Ibanez
2010-11-07 00:35	Luis Ibanez	Note Added: 0022960
2010-11-10 13:06	Sean McBride	Note Added: 0023143
2010-11-11 12:04	Bradley Lowekamp	Note Added: 0023177
2010-11-12 03:35	Gaetan Lehmann	Note Added: 0023196
2010-11-12 12:25	Sean McBride	Note Added: 0023206
2010-11-14 14:03	Luis Ibanez	Assigned To	Luis Ibanez => David Cole
2010-11-15 07:02	David Cole	Note Added: 0023253
2010-11-16 03:50	Gaetan Lehmann	Note Added: 0023280
2010-11-17 10:12	David Cole	Note Added: 0023334
2010-11-17 18:00	David Cole	Note Added: 0023364
2010-11-23 06:37	David Cole	Note Added: 0023518
2011-01-19 10:29	David Cole	Sprint Status	=> backlog
2011-01-19 10:29	David Cole	Note Added: 0024912
2011-01-19 10:29	David Cole	Assigned To	David Cole => Luis Ibanez
2011-01-19 10:47	Sean McBride	Note Added: 0024916
2011-01-20 11:06	Sean McBride	Note Added: 0024959