Port FontDescriptionKey::computeHash() from legacy IntegerHasher to Hasher

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Silence UBSan warning in FontDescriptionKey::computeHash():
- Source/WebCore/platform/graphics/FontCache.h:115:20: runtime error: -5 is outside the range of representable values of type 'unsigned int'
- Source/WebCore/platform/graphics/FontCache.h:115:20: runtime error: -45 is outside the range of representable values of type 'unsigned int'
- Source/WebCore/platform/graphics/FontCache.h:114:20: runtime error: -100 is outside the range of representable values of type 'unsigned int'

[Chris Dumez]

Created attachment 424155 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 424155 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Since we're merely computing a hash here, I think the undefined behavior is acceptable here.

Are you saying that even though it's undefined, all compilers we use will always do a sane thing here? Officially, undefined behavior can erase my SSD, which I wouldn't like a hash function to do :)

[Darin Adler]

Comment on attachment 424155 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:109
> +    NO_SANITIZE("undefined") inline unsigned computeHash() const

I guess this is OK, because IntegerHasher is deprecated and our stated direction is to move to Hasher instead.

If IntegerHasher was not deprecated, I would say that instead we need to do more overloading of the add function in IntegerHasher rather than writing NO_SANITIZE.

Maybe we should port this function to use Hasher instead?

[Darin Adler]

Comment on attachment 424155 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        Since we're merely computing a hash here, I think the undefined behavior is acceptable here.
> 
> Are you saying that even though it's undefined, all compilers we use will always do a sane thing here? Officially, undefined behavior can erase my SSD, which I wouldn't like a hash function to do :)

That’s right.

Converting an int to unsigned without a cast works the same on all compilers, reinterpreting it, even though it’s technically "undefined behavior". This is a problem with UBSan: there are important kinds of problems that are undefined behavior, and there are things that are really no problem at all like this.

[Darin Adler]

Comment on attachment 424155 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.h:109
>> +    NO_SANITIZE("undefined") inline unsigned computeHash() const
> 
> I guess this is OK, because IntegerHasher is deprecated and our stated direction is to move to Hasher instead.
> 
> If IntegerHasher was not deprecated, I would say that instead we need to do more overloading of the add function in IntegerHasher rather than writing NO_SANITIZE.
> 
> Maybe we should port this function to use Hasher instead?

Or we could just add one explicit cast to unsigned.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 424155 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424155&action=review
> 
> >> Source/WebCore/platform/graphics/FontCache.h:109
> >> +    NO_SANITIZE("undefined") inline unsigned computeHash() const
> > 
> > I guess this is OK, because IntegerHasher is deprecated and our stated direction is to move to Hasher instead.
> > 
> > If IntegerHasher was not deprecated, I would say that instead we need to do more overloading of the add function in IntegerHasher rather than writing NO_SANITIZE.
> > 
> > Maybe we should port this function to use Hasher instead?
> 
> Or we could just add one explicit cast to unsigned.

I guess we could port the function to Hasher but all it does is force the caller to static cast to an unsigned integer type before calling Hasher::add(). Will the static_cast<> really silence the UBSan warning? Seems to me that casting a negative int to an unsigned is undefined behavior no matter what.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> casting a negative int to an unsigned is undefined behavior
> no matter what.

There has to be some way to do the conversion that does not rely on undefined behavior. Whether it’s static_cast or a new function we have to write.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> (In reply to Chris Dumez from comment #6)
> > casting a negative int to an unsigned is undefined behavior
> > no matter what.
> 
> There has to be some way to do the conversion that does not rely on
> undefined behavior. Whether it’s static_cast or a new function we have to
> write.

Yes, so I initially tried to write such a function. Then I figured it was probably not worth the cost (I think it will involve extra branching) since this is only used for hashing.

[Darin Adler]

We don’t want to be required to sprinkle NO_SANITIZE("undefined") calls into our sources every place we hash an int.

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> We don’t want to be required to sprinkle NO_SANITIZE("undefined") calls into
> our sources every place we hash an int.

Fair point. Let me rethink the approach then.

[Darin Adler]

Instead we can have that in a single function inside the Hasher class.

It might be harder to do that for IntegerHasher because overloading for int would lead to having to do more overloading now that there is ambiguity.

[Chris Dumez]

Created attachment 424165 [details]
Patch

[Chris Dumez]

I updated the patch to port the code to Hasher as suggested. Set review flag again.

[Darin Adler]

Comment on attachment 424165 [details]
Patch

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

Some nice features of Hasher that this does not take advantage of, that I would like you to be aware of for the future:

1) Can pass an entire collection, like a std::array, and Hasher knows how to hash all the items. Works for any object that has a begin function and that works with a range-based for loop.

2) Can list more than one item in a single call to add, rather than using separate calls to add each thing.

3) Can handle Optional, so no need to write valueOr.

4) Can pass a tuple and it will hash all the items in the tuple.

5) Can hash whole objects, not just integers, so can avoid doing hashes of hashes in most cases.

6) Can write add function overloads for use in combination with the above.

> Source/WebCore/platform/graphics/FontCache.h:115
> +        add(hasher, m_fontSelectionRequest.weight);
> +        add(hasher, m_fontSelectionRequest.width);
> +        add(hasher, m_fontSelectionRequest.slope.valueOr(normalItalicValue()));

Could just write this:

    add(hasher, m_fontSelectionRequest.tied());

> Source/WebCore/platform/graphics/FontCache.h:116
> +        add(hasher, m_locale.existingHash());

Could just write this:

    add(hasher, m_locale);

> Source/WebCore/platform/graphics/FontCache.h:117
>          for (unsigned flagItem : m_flags)

Could just write this:

    add(hasher, m_flags);

Instead of the loop.

> Source/WebCore/platform/graphics/FontCache.h:120
> +        add(hasher, m_featureSettings.hash());
> +        add(hasher, m_variationSettings.hash());

With very little work we could refactor so this doesn’t make a hash out of hashes. Likely just need to make an add(Hasher&, x) overload for FontTaggedSetting.

No need to do that now.

[Chris Dumez]

Created attachment 424209 [details]
Patch

[EWS]

Committed r274992: <https://commits.webkit.org/r274992>

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

[Radar WebKit Bug Importer]

<rdar://problem/75815864>

[Ryosuke Niwa]

This patch caused the bug 223858.