Compile warning (-Wsign-compare) on 32-bits at WebCore/platform/FileSystem.cpp
authoryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Aug 2015 10:05:40 +0000 (10:05 +0000)
committeryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Aug 2015 10:05:40 +0000 (10:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=146414

Reviewed by Darin Adler.

Source/WebCore:

No behavioral changes.

* platform/FileSystem.cpp:
(WebCore::MappedFileData::MappedFileData): Making use of convertSafely.
* platform/posix/SharedBufferPOSIX.cpp:
(WebCore::SharedBuffer::createFromReadingFile): Making use of convertSafely.

Source/WTF:

Added convertSafely routine based on isInBounds routine.
Updated BoundChecker by adding a third boolean parameter to this template giving whether Target has greater or equal precision than Source.
Removed BoundCheckElider, which is no longer necessary and had some issues.

* wtf/CheckedArithmetic.h:
(WTF::isInBounds):
(WTF::convertSafely):

Tools:

* TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:
(TestWebKitAPI::TEST): Improving testing of WTF::isInBounds.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@188210 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/CheckedArithmetic.h
Source/WebCore/ChangeLog
Source/WebCore/platform/FileSystem.cpp
Source/WebCore/platform/posix/SharedBufferPOSIX.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp

index ebe0db1..1254b85 100644 (file)
@@ -1,3 +1,18 @@
+2015-08-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        Compile warning (-Wsign-compare) on 32-bits at WebCore/platform/FileSystem.cpp
+        https://bugs.webkit.org/show_bug.cgi?id=146414
+
+        Reviewed by Darin Adler.
+
+        Added convertSafely routine based on isInBounds routine.
+        Updated BoundChecker by adding a third boolean parameter to this template giving whether Target has greater or equal precision than Source.
+        Removed BoundCheckElider, which is no longer necessary and had some issues.
+
+        * wtf/CheckedArithmetic.h:
+        (WTF::isInBounds):
+        (WTF::convertSafely):
+
 2015-08-07  Filip Pizlo  <fpizlo@apple.com>
 
         Lightweight locks should be adaptive
 2015-08-07  Filip Pizlo  <fpizlo@apple.com>
 
         Lightweight locks should be adaptive
index 6cc45b3..0893a87 100644 (file)
@@ -122,63 +122,84 @@ template <typename T, class OverflowHandler = CrashOnOverflow> class Checked;
 template <typename T> struct RemoveChecked;
 template <typename T> struct RemoveChecked<Checked<T>>;
 
 template <typename T> struct RemoveChecked;
 template <typename T> struct RemoveChecked<Checked<T>>;
 
-template <typename Target, typename Source, bool targetSigned = std::numeric_limits<Target>::is_signed, bool sourceSigned = std::numeric_limits<Source>::is_signed> struct BoundsChecker;
-template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, false> {
+template <typename Target, typename Source, bool isTargetBigger = sizeof(Target) >= sizeof(Source), bool targetSigned = std::numeric_limits<Target>::is_signed, bool sourceSigned = std::numeric_limits<Source>::is_signed> struct BoundsChecker;
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, false, false> {
     static bool inBounds(Source value)
     {
     static bool inBounds(Source value)
     {
-        // Same signedness so implicit type conversion will always increase precision
-        // to widest type
+        // Same signedness so implicit type conversion will always increase precision to widest type.
         return value <= std::numeric_limits<Target>::max();
     }
 };
         return value <= std::numeric_limits<Target>::max();
     }
 };
-
-template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, true> {
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, true, true> {
     static bool inBounds(Source value)
     {
     static bool inBounds(Source value)
     {
-        // Same signedness so implicit type conversion will always increase precision
-        // to widest type
+        // Same signedness so implicit type conversion will always increase precision to widest type.
         return std::numeric_limits<Target>::min() <= value && value <= std::numeric_limits<Target>::max();
     }
 };
 
         return std::numeric_limits<Target>::min() <= value && value <= std::numeric_limits<Target>::max();
     }
 };
 
-template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, true> {
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, false, true> {
     static bool inBounds(Source value)
     {
     static bool inBounds(Source value)
     {
-        // Target is unsigned so any value less than zero is clearly unsafe
-        if (value < 0)
-            return false;
-        // If our (unsigned) Target is the same or greater width we can
-        // convert value to type Target without losing precision
-        if (sizeof(Target) >= sizeof(Source)) 
-            return static_cast<Target>(value) <= std::numeric_limits<Target>::max();
-        // The signed Source type has greater precision than the target so
-        // max(Target) -> Source will widen.
-        return value <= static_cast<Source>(std::numeric_limits<Target>::max());
+        // When converting value to unsigned Source, value will become a big value if value is negative.
+        // Casted value will become bigger than Target::max as Source is bigger than Target.
+        return static_cast<typename std::make_unsigned<Source>::type>(value) <= std::numeric_limits<Target>::max();
     }
 };
 
     }
 };
 
-template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, false> {
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, false, true, false> {
     static bool inBounds(Source value)
     {
     static bool inBounds(Source value)
     {
-        // Signed target with an unsigned source
-        if (sizeof(Target) <= sizeof(Source)) 
-            return value <= static_cast<Source>(std::numeric_limits<Target>::max());
-        // Target is Wider than Source so we're guaranteed to fit any value in
-        // unsigned Source
+        // The unsigned Source type has greater precision than the target so max(Target) -> Source will widen.
+        return value <= static_cast<Source>(std::numeric_limits<Target>::max());
+    }
+};
+
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, false, false> {
+    static bool inBounds(Source)
+    {
+        // Same sign, greater or same precision.
         return true;
     }
 };
 
         return true;
     }
 };
 
-template <typename Target, typename Source, bool CanElide = std::is_same<Target, Source>::value || (sizeof(Target) > sizeof(Source)) > struct BoundsCheckElider;
-template <typename Target, typename Source> struct BoundsCheckElider<Target, Source, true> {
-    static bool inBounds(Source) { return true; }
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, true, true> {
+    static bool inBounds(Source)
+    {
+        // Same sign, greater or same precision.
+        return true;
+    }
 };
 };
-template <typename Target, typename Source> struct BoundsCheckElider<Target, Source, false> : public BoundsChecker<Target, Source> {
+
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, true, false> {
+    static bool inBounds(Source value)
+    {
+        // Target is signed with greater or same precision. If strictly greater, it is always safe.
+        if (sizeof(Target) > sizeof(Source))
+            return true;
+        return value <= static_cast<Source>(std::numeric_limits<Target>::max());
+    }
+};
+
+template <typename Target, typename Source> struct BoundsChecker<Target, Source, true, false, true> {
+    static bool inBounds(Source value)
+    {
+        // Target is unsigned with greater precision.
+        return value >= 0;
+    }
 };
 
 template <typename Target, typename Source> static inline bool isInBounds(Source value)
 {
 };
 
 template <typename Target, typename Source> static inline bool isInBounds(Source value)
 {
-    return BoundsCheckElider<Target, Source>::inBounds(value);
+    return BoundsChecker<Target, Source>::inBounds(value);
+}
+
+template <typename Target, typename Source> static inline bool convertSafely(Source input, Target& output)
+{
+    if (!isInBounds<Target>(input))
+        return false;
+    output = static_cast<Target>(input);
+    return true;
 }
 
 template <typename T> struct RemoveChecked {
 }
 
 template <typename T> struct RemoveChecked {
index 89dcf19..ba8ea50 100644 (file)
@@ -1,5 +1,19 @@
 2015-08-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
 2015-08-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
+        Compile warning (-Wsign-compare) on 32-bits at WebCore/platform/FileSystem.cpp
+        https://bugs.webkit.org/show_bug.cgi?id=146414
+
+        Reviewed by Darin Adler.
+
+        No behavioral changes.
+
+        * platform/FileSystem.cpp:
+        (WebCore::MappedFileData::MappedFileData): Making use of convertSafely.
+        * platform/posix/SharedBufferPOSIX.cpp:
+        (WebCore::SharedBuffer::createFromReadingFile): Making use of convertSafely.
+
+2015-08-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
         [Streams API] ReadableStreamReader closed promise should use CachedAttribute
         https://bugs.webkit.org/show_bug.cgi?id=147487
 
         [Streams API] ReadableStreamReader closed promise should use CachedAttribute
         https://bugs.webkit.org/show_bug.cgi?id=147487
 
index 946d5f3..87e3053 100644 (file)
@@ -151,14 +151,13 @@ MappedFileData::MappedFileData(const String& filePath, bool& success)
         return;
     }
 
         return;
     }
 
-    if (fileStat.st_size < 0 || fileStat.st_size > std::numeric_limits<unsigned>::max()) {
+    unsigned size;
+    if (!WTF::convertSafely(fileStat.st_size, size)) {
         close(fd);
         success = false;
         return;
     }
 
         close(fd);
         success = false;
         return;
     }
 
-    unsigned size = static_cast<unsigned>(fileStat.st_size);
-
     if (!size) {
         close(fd);
         success = true;
     if (!size) {
         close(fd);
         success = true;
index 863dbe2..7e9a026 100644 (file)
@@ -51,8 +51,8 @@ RefPtr<SharedBuffer> SharedBuffer::createFromReadingFile(const String& filePath)
         return 0;
     }
 
         return 0;
     }
 
-    size_t bytesToRead = fileStat.st_size;
-    if (fileStat.st_size < 0 || bytesToRead != static_cast<unsigned long long>(fileStat.st_size)) {
+    size_t bytesToRead;
+    if (!WTF::convertSafely(fileStat.st_size, bytesToRead)) {
         close(fd);
         return 0;
     }
         close(fd);
         return 0;
     }
index 3d4d1a6..d41aa32 100644 (file)
@@ -1,3 +1,13 @@
+2015-08-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        Compile warning (-Wsign-compare) on 32-bits at WebCore/platform/FileSystem.cpp
+        https://bugs.webkit.org/show_bug.cgi?id=146414
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:
+        (TestWebKitAPI::TEST): Improving testing of WTF::isInBounds.
+
 2015-08-10  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] Test  /webkit2/WebKitWebView/submit-form is flaky
 2015-08-10  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] Test  /webkit2/WebKitWebView/submit-form is flaky
index efdcbc7..d6b5483 100644 (file)
@@ -426,4 +426,62 @@ CheckedArithmeticTest(uint32_t, CoerceLiteralToUnsigned, AllowMixedSignednessTes
 CheckedArithmeticTest(int64_t, CoerceLiteralNop, IgnoreMixedSignednessTest)
 CheckedArithmeticTest(uint64_t, CoerceLiteralToUnsigned, IgnoreMixedSignednessTest)
 
 CheckedArithmeticTest(int64_t, CoerceLiteralNop, IgnoreMixedSignednessTest)
 CheckedArithmeticTest(uint64_t, CoerceLiteralToUnsigned, IgnoreMixedSignednessTest)
 
+TEST(CheckedArithmeticTest, IsInBounds)
+{
+    // bigger precision, signed, signed
+    EXPECT_TRUE(WTF::isInBounds<int32_t>(std::numeric_limits<int16_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<int32_t>(std::numeric_limits<int16_t>::min()));
+
+    // bigger precision, unsigned, signed
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>(std::numeric_limits<int32_t>::max()));
+    EXPECT_FALSE(WTF::isInBounds<uint32_t>(std::numeric_limits<int16_t>::min()));
+
+    EXPECT_FALSE(WTF::isInBounds<uint32_t>((int32_t)-1));
+    EXPECT_FALSE(WTF::isInBounds<uint16_t>((int32_t)-1));
+    EXPECT_FALSE(WTF::isInBounds<unsigned long>((int)-1));
+
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>((int32_t)1));
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>((int16_t)1));
+    EXPECT_TRUE(WTF::isInBounds<unsigned>((int)1));
+
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>((int32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<uint16_t>((int32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>((int16_t)0));
+    EXPECT_TRUE(WTF::isInBounds<unsigned>((int)0));
+
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>(std::numeric_limits<int32_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>(std::numeric_limits<int16_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<unsigned>(std::numeric_limits<int>::max()));
+
+    // bigger precision, signed, unsigned
+    EXPECT_TRUE(WTF::isInBounds<int32_t>(std::numeric_limits<uint16_t>::max()));
+    EXPECT_FALSE(WTF::isInBounds<int32_t>(std::numeric_limits<uint32_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<int32_t>((uint32_t)0));
+
+    // bigger precision, unsigned, unsigned
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>(std::numeric_limits<uint16_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<uint32_t>(std::numeric_limits<uint16_t>::min()));
+
+    // lower precision, signed signed
+    EXPECT_FALSE(WTF::isInBounds<int16_t>(std::numeric_limits<int32_t>::max()));
+    EXPECT_FALSE(WTF::isInBounds<int16_t>(std::numeric_limits<int32_t>::min()));
+    EXPECT_TRUE(WTF::isInBounds<int16_t>((int32_t)-1));
+    EXPECT_TRUE(WTF::isInBounds<int16_t>((int32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<int16_t>((int32_t)1));
+    // lower precision, unsigned, signed
+    EXPECT_FALSE(WTF::isInBounds<uint16_t>(std::numeric_limits<int32_t>::max()));
+    EXPECT_FALSE(WTF::isInBounds<uint16_t>(std::numeric_limits<int32_t>::min()));
+    EXPECT_FALSE(WTF::isInBounds<uint16_t>((int32_t)-1));
+    EXPECT_TRUE(WTF::isInBounds<uint16_t>((int32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<uint16_t>((int32_t)1));
+    // lower precision, signed, unsigned
+    EXPECT_FALSE(WTF::isInBounds<int16_t>(std::numeric_limits<uint32_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<int16_t>((uint32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<int16_t>((uint32_t)1));
+    // lower precision, unsigned, unsigned
+    EXPECT_FALSE(WTF::isInBounds<uint16_t>(std::numeric_limits<uint32_t>::max()));
+    EXPECT_TRUE(WTF::isInBounds<uint16_t>((uint32_t)0));
+    EXPECT_TRUE(WTF::isInBounds<uint16_t>((uint32_t)1));
+}
+
 } // namespace TestWebKitAPI
 } // namespace TestWebKitAPI