MantisBT - ITK
View Issue Details
0006196ITKpublic2007-12-21 14:382011-01-20 11:06
Sean McBride 
Luis Ibanez 
normalminoralways
assignedopen 
Mac OS XMac OS X10.5
 
 
backlog
0006196: ITK has many 64 to 32 bit truncations, compiler warning can catch many of them
Apple's version of gcc (unlike the mainline) has a warning: -Wshorten-64-to-32

which warns if a value is implicitly converted from a 64 bit type to a 32 bit type.

With this warning enabled, building ITK as either 32 bit or 64 bit results in _many_ warnings. This can be very serious if the 64 bit value happens to hold a value too big for a 32 bit variable.

I would like to see all these warnings fixed, but I know it will take a long time.

Attached are all the warnings as of today's CVS.

Because there are literally thousands of these warnings, it will be best to proceed a little bit at a time. We can start by enabling the warning only in /Common by adding the following to Code/Common/CMakeLists.txt

 IF (APPLE)
   SET_TARGET_PROPERTIES(ITKCommon PROPERTIES COMPILE_FLAGS "-Wshorten-64-to-32")
 ENDIF (APPLE)
No tags attached.
related to 0005529closed kentwilliams ITK warnings in XCode about 64 bits to 32 bits conversion. 
zip ITK64bitwarnings.txt.zip (878,575) 2007-12-21 14:38
https://public.kitware.com/Bug/file/1267/ITK64bitwarnings.txt.zip
Issue History
2007-12-21 14:38Sean McBrideNew Issue
2007-12-21 14:38Sean McBrideFile Added: ITK64bitwarnings.txt.zip
2008-01-03 15:00Sean McBrideDescription Updated
2008-01-18 13:24Luis IbanezNote Added: 0010193
2008-01-18 14:07Luis IbanezNote Added: 0010198
2008-01-18 17:13Sean McBrideNote Added: 0010199
2008-01-22 10:29Sean McBrideNote Added: 0010212
2008-01-23 10:30Sean McBrideNote Added: 0010224
2008-01-23 16:39Luis IbanezRelationship addedrelated to 0005529
2008-01-23 16:39Luis IbanezStatusnew => assigned
2008-01-23 16:39Luis IbanezAssigned To => Luis Ibanez
2010-11-07 00:35Luis IbanezNote Added: 0022960
2010-11-10 13:06Sean McBrideNote Added: 0023143
2010-11-11 12:04Bradley LowekampNote Added: 0023177
2010-11-12 03:35Gaetan LehmannNote Added: 0023196
2010-11-12 12:25Sean McBrideNote Added: 0023206
2010-11-14 14:03Luis IbanezAssigned ToLuis Ibanez => David Cole
2010-11-15 07:02David ColeNote Added: 0023253
2010-11-16 03:50Gaetan LehmannNote Added: 0023280
2010-11-17 10:12David ColeNote Added: 0023334
2010-11-17 18:00David ColeNote Added: 0023364
2010-11-23 06:37David ColeNote Added: 0023518
2011-01-19 10:29David ColeSprint Status => backlog
2011-01-19 10:29David ColeNote Added: 0024912
2011-01-19 10:29David ColeAssigned ToDavid Cole => Luis Ibanez
2011-01-19 10:47Sean McBrideNote Added: 0024916
2011-01-20 11:06Sean McBrideNote Added: 0024959

Notes
(0010193)
Luis Ibanez   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
2010-11-16 03:50   
I've added the flag to the build SunOS-CC-amd64.
It should appear tomorrow.
(0023334)
David Cole   
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   
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   
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   
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   
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   
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.