Bug 146414 – Compile warning (-Wsign-compare) on 32-bits at WebCore/platform/FileSystem.cpp

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

When building WebKitGTK+ on Linux/32-bits the following warning appears:

[2644/5687] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/FileSystem.cpp.o
../../Source/WebCore/platform/FileSystem.cpp: In constructor ‘WebCore::MappedFileData::MappedFileData(const WTF::String&, bool&)’:
../../Source/WebCore/platform/FileSystem.cpp:154:50: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (fileStat.st_size < 0 || fileStat.st_size > std::numeric_limits<unsigned>::max()) {
                                                  ^

https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/53808/steps/compile-webkit/logs/stdio


bug 101458 can give some hints about the issue.

[youenn fablet]

Created attachment 256568 [details]
Patch

[youenn fablet]

(In reply to comment #0)
> When building WebKitGTK+ on Linux/32-bits the following warning appears:

I have not tested it on 32-bits systems but I hope the uploaded patch removes the warning.

[Darin Adler]

Comment on attachment 256568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256568&action=review

> Source/WebCore/platform/FileSystem.cpp:154
> +    if (fileStat.st_size < 0 || static_cast<size_t>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {

If we got the typecast to exactly the right unsigned type, one guaranteed to be the same size as off_t, then the check against 0 becomes dead code, because negative numbers are guaranteed to turn into large unsigned numbers. It’s not obvious to me that size_t is guaranteed to be the right type. I would write this:

    if (static_cast<std::make_unsigned<off_t>::type>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {

[Darin Adler]

Comment on attachment 256568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256568&action=review

>> Source/WebCore/platform/FileSystem.cpp:154
>> +    if (fileStat.st_size < 0 || static_cast<size_t>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {
> 
> If we got the typecast to exactly the right unsigned type, one guaranteed to be the same size as off_t, then the check against 0 becomes dead code, because negative numbers are guaranteed to turn into large unsigned numbers. It’s not obvious to me that size_t is guaranteed to be the right type. I would write this:
> 
>     if (static_cast<std::make_unsigned<off_t>::type>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {

Here’s an even more bulletproof way to write this:

    template<typename T> static inline makeUnsigned(T integer) { return static_cast<std::make_unsigned<T>::type>(integer); }

    if (makeUnsigned(fileState.st_size) > std::numeric_limits<unsigned>::max()) {

[youenn fablet]

Comment on attachment 256568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256568&action=review

>>> Source/WebCore/platform/FileSystem.cpp:154
>>> +    if (fileStat.st_size < 0 || static_cast<size_t>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {
>> 
>> If we got the typecast to exactly the right unsigned type, one guaranteed to be the same size as off_t, then the check against 0 becomes dead code, because negative numbers are guaranteed to turn into large unsigned numbers. It’s not obvious to me that size_t is guaranteed to be the right type. I would write this:
>> 
>>     if (static_cast<std::make_unsigned<off_t>::type>(fileStat.st_size) > std::numeric_limits<unsigned>::max()) {
> 
> Here’s an even more bulletproof way to write this:
> 
>     template<typename T> static inline makeUnsigned(T integer) { return static_cast<std::make_unsigned<T>::type>(integer); }
> 
>     if (makeUnsigned(fileState.st_size) > std::numeric_limits<unsigned>::max()) {

If we want to further improve readability, we might want to write "if (!WTF::isInBounds<unsigned>(fileState.st_size))"
Integrating your suggestion in WTF::isInBounds might be useful then.

[youenn fablet]

Created attachment 256678 [details]
WTF::isInBounds version

[youenn fablet]

Comment on attachment 256678 [details]
WTF::isInBounds version

Putting r?
This patch is missing unit tests to ensure everything is working fine.
I will do that when going back to WebKit in a few weeks.

[youenn fablet]

Created attachment 257833 [details]
Patch

[youenn fablet]

(In reply to comment #8)
> Created attachment 257833 [details]
> Patch

Adding unit tests and adding a small fix to BoundsCheckElider.

[Oliver Hunt]

Comment on attachment 257833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257833&action=review

> Source/WebCore/platform/FileSystem.cpp:154
> -    if (fileStat.st_size < 0 || fileStat.st_size > std::numeric_limits<unsigned>::max()) {
> +    if (!WTF::isInBounds<unsigned>(fileStat.st_size)) {

Random question, why unsigned here?

> Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:434
> +    EXPECT_FALSE(WTF::isInBounds<uint32_t>((int32_t)-1));
> +    EXPECT_FALSE(WTF::isInBounds<uint16_t>((int32_t)-1));
> +    EXPECT_FALSE(WTF::isInBounds<uint32_t>((int16_t)-1));

Can you also add tests for 1, and max<int32,uint32,int16,...> etc? and verify that there hasn't been any behaviour change?

[youenn fablet]

Comment on attachment 257833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257833&action=review

>> Source/WebCore/platform/FileSystem.cpp:154
>> +    if (!WTF::isInBounds<unsigned>(fileStat.st_size)) {
> 
> Random question, why unsigned here?

We do a conversion from off_t to unsigned just after the "if", unsigned being what is expected by SharedBuffer at the end.
It might be nice to refactor this in a WTF::convertSafely routine that would do the inbound check as well as the static_cast.

>> Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:434
>> +    EXPECT_FALSE(WTF::isInBounds<uint32_t>((int16_t)-1));
> 
> Can you also add tests for 1, and max<int32,uint32,int16,...> etc? and verify that there hasn't been any behaviour change?

OK

[youenn fablet]

Created attachment 257908 [details]
Trying to fix build, adding tests and convertSafely

[youenn fablet]

Created attachment 258060 [details]
Another try at fixing mac build

[youenn fablet]

Created attachment 258062 [details]
Fixing EFL build

[WebKit Commit Bot]

Comment on attachment 258062 [details]
Fixing EFL build

Clearing flags on attachment: 258062

Committed r188210: <http://trac.webkit.org/changeset/188210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.