Bug 224797 – [clang] Remove implicit cast related warnings.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

WTF/Headers\wtf/CheckedArithmetic.h:836:58: warning: implicit conversion from 'std::numeric_limits<unsigned long>::_Ty' (aka 'unsigned long') to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Wimplicit-const-int-float-conversion]
        if (!(std::numeric_limits<T>::min() <= result && std::numeric_limits<T>::max() >= result))
                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~
../../Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:511:10: note: in instantiation of member function 'WTF::Checked<uint64_t, WTF::RecordOverflow>::operator/=' requested here
    size /= 10.5;
         ^
1 warning generated.

In file included from WebCore/DerivedSources/unified-sources/UnifiedSource-043dd90b-5.cpp:8:
../../Source/WebCore\rendering/RenderBlockFlow.cpp:1785:25: warning: implicit conversion from 'const int' to 'const float' changes value from 33554431 to 33554432 [-Wimplicit-const-int-float-conversion]
        logicalOffset = intMaxForLayoutUnit;
                      ~ ^~~~~~~~~~~~~~~~~~~
1 warning generated.

[Basuke Suzuki]

Created attachment 426513 [details]
PATCH

[Basuke Suzuki]

Checked::operator /= () is only used in this TestWTF file and using long double wont affect in actual codebase.

[Darin Adler]

Comment on attachment 426513 [details]
PATCH

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

> Source/WTF/wtf/CheckedArithmetic.h:820
> +        long double result = rhs * m_value;

This looks like a costly solution to this problem, and possibly not portable. Is there a way to do this checking without involving a "long double".

> Source/WebCore/ChangeLog:10
> +        Added explicit cast for compatibility.
> +
> +        No new tests because there's no practical change.

I don’t understand what "compatibility" means here. Compatibility with what?

[Basuke Suzuki]

Comment on attachment 426513 [details]
PATCH

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

>> Source/WTF/wtf/CheckedArithmetic.h:820
>> +        long double result = rhs * m_value;
> 
> This looks like a costly solution to this problem, and possibly not portable. Is there a way to do this checking without involving a "long double".

As the warning says: "'unsigned long') to 'double' changes value from 18446744073709551615 to 18446744073709551616", double is not as precise as to hold unsigned long. What if the value is 18446744073709551616, then the value will pass through this check and won't overflow. I cannot judge what range of error can be allowed from this generic implementation, so that it has to be strict. But I didn't think about the portability. Humm, I don't have other solution right now. Let's think about this.

>> Source/WebCore/ChangeLog:10
>> +        No new tests because there's no practical change.
> 
> I don’t understand what "compatibility" means here. Compatibility with what?

I mean it it compatible with the result of implicit cast with explicit cast. That's why technically there's no change in the compilation result.

[Darin Adler]

Comment on attachment 426513 [details]
PATCH

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

>>> Source/WTF/wtf/CheckedArithmetic.h:820
>>> +        long double result = rhs * m_value;
>> 
>> This looks like a costly solution to this problem, and possibly not portable. Is there a way to do this checking without involving a "long double".
> 
> As the warning says: "'unsigned long') to 'double' changes value from 18446744073709551615 to 18446744073709551616", double is not as precise as to hold unsigned long. What if the value is 18446744073709551616, then the value will pass through this check and won't overflow. I cannot judge what range of error can be allowed from this generic implementation, so that it has to be strict. But I didn't think about the portability. Humm, I don't have other solution right now. Let's think about this.

Comment above says "unsigned long" but this is about 64-bit unsigned integers; "unsigned long" is a 32-bit integer.

- We don’t want to use long double when the underlying type is a 32-bit integer. That would hurt efficiency in that case without helping correctness.
- We need to add test cases that demonstrate this working incorrectly before we start making changes that aren’t tested.
- Do we actually use these function with uint64_t? Instead of making this less efficient and less portable for cases we actually do use, maybe instead we should find a way to make this not compile at all when T is 64-bit for now, while we seek a better result.

[Basuke Suzuki]

(In reply to Darin Adler from comment #5)
> Comment on attachment 426513 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426513&action=review
> 
> >>> Source/WTF/wtf/CheckedArithmetic.h:820
> >>> +        long double result = rhs * m_value;
> >> 
> >> This looks like a costly solution to this problem, and possibly not portable. Is there a way to do this checking without involving a "long double".
> > 
> > As the warning says: "'unsigned long') to 'double' changes value from 18446744073709551615 to 18446744073709551616", double is not as precise as to hold unsigned long. What if the value is 18446744073709551616, then the value will pass through this check and won't overflow. I cannot judge what range of error can be allowed from this generic implementation, so that it has to be strict. But I didn't think about the portability. Humm, I don't have other solution right now. Let's think about this.
> 
> Comment above says "unsigned long" but this is about 64-bit unsigned
> integers; "unsigned long" is a 32-bit integer.

Is unsigned long 32-bit on Mac?? It's usually 64-bit on 64-bit architecture. Actually it is Checked<size_t> so it has to be 64-bit.

> - We don’t want to use long double when the underlying type is a 32-bit
> integer. That would hurt efficiency in that case without helping correctness.

Agreed. We need to add such case.

> - We need to add test cases that demonstrate this working incorrectly before
> we start making changes that aren’t tested.

Ditto.

> - Do we actually use these function with uint64_t? Instead of making this
> less efficient and less portable for cases we actually do use, maybe instead
> we should find a way to make this not compile at all when T is 64-bit for
> now, while we seek a better result.

Okay. I'll create a new bug for that. Removing that from this path.

[Darin Adler]

(In reply to Basuke Suzuki from comment #6)
> Is unsigned long 32-bit on Mac??

Yes.

> It's usually 64-bit on 64-bit architecture.

Maybe often, but not "usually".

https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models

Windows and all Apple platforms use 32-bit longs (LLP64). I understand that on many Unix platforms the compilers are configured for 64-bit longs (LP64). In part because of this we avoid using the type "long" and "unsigned long" almost all the time in WebKit, because the code would just end up being non-portable. Almost any place it’s used, we should be using "int" or "unsigned" instead; this often happens because of confusion since IDL uses "long" and "unsigned long" for the 32-bit types.

> Actually it is Checked<size_t> so it has to be 64-bit.

OK.

On further reflection, it’s possible that long double *is* portable enough in practice for us to use it for this purpose. It’s not guaranteed to be big enough to hold all 64-bit integers, but in practice it probably is on all the platforms we support.

This header originally came from another project. Maybe Mozilla? We might want to check how they solved the problem.

[Basuke Suzuki]

Filed. https://bugs.webkit.org/show_bug.cgi?id=224835

[Basuke Suzuki]

Created attachment 426615 [details]
PATCH

[EWS]

Committed r276342 (236820@main): <https://commits.webkit.org/236820@main>

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

[Basuke Suzuki]

(In reply to Darin Adler from comment #7)
> (In reply to Basuke Suzuki from comment #6)
> > Is unsigned long 32-bit on Mac??
> 
> Yes.

Darin, I wrote a test code and ran on M1 MacBook Air both native and Rosetta.
It both says unsigned long is eight byte.

https://gist.github.com/basuke/63dff61dcaf2096f88449408aed90781

So it seems Checked with 64-bit type is already used widely on the codebase.

[Darin Adler]

Embarrassing that I got that wrong! I was totally incorrect.

It’s still true that we mostly avoid the type "unsigned long" in our code because of the portability problems on 64-bit Windows, for example.

What files did you find where we were using Checked<unsigned long> (as opposed to Checked<size_t> or Checked<uint64_t>?

[Radar WebKit Bug Importer]

<rdar://problem/77061502>

[Basuke Suzuki]

(In reply to Darin Adler from comment #12)
> Embarrassing that I got that wrong! I was totally incorrect.
> 
> It’s still true that we mostly avoid the type "unsigned long" in our code
> because of the portability problems on 64-bit Windows, for example.
> 
> What files did you find where we were using Checked<unsigned long> (as
> opposed to Checked<size_t> or Checked<uint64_t>?

Oh, sorry. No, it's not in the code. The compiler deduced the Checked<size_t> to Checked<unsigned long> in the warning message. It's not in the code.

Regardless of this, https://bugs.webkit.org/show_bug.cgi?id=224835 is the actual issue we need to solve.

[Darin Adler]

Agreed.