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.
Created attachment 256568 [details] Patch
(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.
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()) {
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()) {
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.
Created attachment 256678 [details] WTF::isInBounds version
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.
Created attachment 257833 [details] Patch
(In reply to comment #8) > Created attachment 257833 [details] > Patch Adding unit tests and adding a small fix to BoundsCheckElider.
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?
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
Created attachment 257908 [details] Trying to fix build, adding tests and convertSafely
Created attachment 258060 [details] Another try at fixing mac build
Created attachment 258062 [details] Fixing EFL build
Comment on attachment 258062 [details] Fixing EFL build Clearing flags on attachment: 258062 Committed r188210: <http://trac.webkit.org/changeset/188210>
All reviewed patches have been landed. Closing bug.