Bug 213383 – -Wsign-compare in isValidOptionSet

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Michael Catanzaro]

Since r263208 "[IPC hardening] OptionSet<> values should be validated":

[4332/4574] Building CXX object Source...ources/UnifiedSource-50d0d8dd-14.cpp.o
In file included from /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.h:31,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.cpp:27,
                 from DerivedSources/WebKit/unified-sources/UnifiedSource-50d0d8dd-14.cpp:1:
DerivedSources/ForwardingHeaders/wtf/OptionSet.h: In instantiation of ‘constexpr bool WTF::isValidOptionSet(WTF::OptionSet<E>) [with E = WebKit::InputMethodState::Hint]’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoders.h:69:35:   required from ‘static bool IPC::ArgumentCoder<WTF::OptionSet<E> >::decode(IPC::Decoder&, WTF::OptionSet<E>&) [with T = WebKit::InputMethodState::Hint]’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoder.h:72:41:   required by substitution of ‘template<class T> static uint8_t IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::checkArgumentCoder<T>(IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::Helper<bool (*)(IPC::Decoder&, WTF::OptionSet<WebKit::InputMethodState::Hint>&), (& IPC::ArgumentCoder<T>::decode)>*) [with T = WTF::OptionSet<WebKit::InputMethodState::Hint>]’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoder.h:76:85:   required from ‘constexpr const bool IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::value’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/Decoder.h:140:126:   required by substitution of ‘template<class T, std::enable_if_t<(((! std::is_enum<_Tp>::value) && (! std::is_arithmetic<_Tp>::value)) && (! IPC::UsesLegacyDecoder<U>::value))>* <anonymous> > bool IPC::Decoder::decode(T&) [with T = WTF::OptionSet<WebKit::InputMethodState::Hint>; std::enable_if_t<(((! std::is_enum<_Tp>::value) && (! std::is_arithmetic<_Tp>::value)) && (! IPC::UsesLegacyDecoder<U>::value))>* <anonymous> = <missing>]’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.cpp:117:36:   required from here
DerivedSources/ForwardingHeaders/wtf/OptionSet.h:257:52: warning: comparison of integer expressions of different signedness: ‘WTF::OptionSet<WebKit::InputMethodState::Hint>::StorageType’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare]
  257 |     return (optionSet.toRaw() | allValidBitsValue) == allValidBitsValue;
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

[Michael Catanzaro]

Since this is a bitwise comparison, and we don't normally want to use signed types for bitwise comparisons, we probably want to cast allValidBitsValue on the right side of the expression to unsigned. I think that can be done using std::make_unsigned. Testing this....

[Michael Catanzaro]

So that does work, but I'm going to try to mandate use of unsigned type instead. Using signed types with OptionSet is probably a mistake.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> So that does work, but I'm going to try to mandate use of unsigned type
> instead. Using signed types with OptionSet is probably a mistake.

So that was a bad idea, because OptionSet goes out of its way to allow this:

using StorageType = std::make_unsigned_t<std::underlying_type_t<E>>;

I think isValidOptionSet should do the same. Let me try that.

[Michael Catanzaro]

OK, I present two... options. ;)

Option #1: OptionSetValueChecker should match the OptionSet's StorageType, which is std::make_unsigned_t<std::underlying_type_t<E>>, instead of directly using std::underlying_type_t<E>:

diff --git a/Source/WTF/wtf/OptionSet.h b/Source/WTF/wtf/OptionSet.h
index de111b16b355..8450b1a240d9 100644
--- a/Source/WTF/wtf/OptionSet.h
+++ b/Source/WTF/wtf/OptionSet.h
@@ -253,8 +253,8 @@ private:
 template<typename E>
 WARN_UNUSED_RETURN constexpr bool isValidOptionSet(OptionSet<E> optionSet)
 {
-    auto allValidBitsValue = OptionSetValueChecker<std::underlying_type_t<E>, typename EnumTraits<E>::values>::allValidBits();
-    return (optionSet.toRaw() | allValidBitsValue) == allValidBitsValue;
+    auto allValidBitsValue = OptionSetValueChecker<std::make_unsigned_t<std::underlying_type_t<E>>, typename EnumTraits<E>::values>::allValidBits();
+    return (optionSet.toRaw() | allValidBitsValue) == std::underlying_type_t<E>(allValidBitsValue);
 }

That fixes the warning without making any changes in other files. I'll attach this patch for review.

Option #2: we could require the OptionSet to use an unsigned underlying value to call isValidOptionSet using std::enable_if. That would enforce unsigned underlying value only for OptionSets that are validated by isValidOptionSet. We only have to change the underlying type of WebKit::InputMethodState::Hint in Source/WebKit/Shared/glib/InputMethodState.h. I decided against making this my preferred option because valid OptionSets do not actually have to use unsigned underlying storage, so it would be a bit weird to require that to call isValidOptionSet, but this would be useful if we want to enforce this for types sent over IPC. (Do we want to enforce it for types sent over IPC?)

Option #3: would be to prohibit OptionSet from ever using signed underlying value, but that's going to require changing a lot of enums. Doesn't seem worth it, so I don't propose this.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> +    return (optionSet.toRaw() | allValidBitsValue) ==
> std::underlying_type_t<E>(allValidBitsValue);

That's a Ctrl+Z mistake, I'll attach a patch that actually works.

[Michael Catanzaro]

Created attachment 402301 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> this would be useful if we want to enforce this
> for types sent over IPC. (Do we want to enforce it for types sent over IPC?)

Maybe we do? r263208 ensured this for all types used in cross-platform code. If it's important, r- my patch and I'll upload a version that enforces this. The downside is that it would no longer be possible to call isValidOptionSet on valid OptionSets that use signed underlying storage, which would be weird.

[EWS]

Committed r263280: <https://trac.webkit.org/changeset/263280>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402301 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64541629>

[David Kilzer (:ddkilzer)]

Thanks Michael!