RFR: 8314258: checked_cast doesn't properly check some cases

[Kim Barrett]

On Sat, 14 Oct 2023 19:29:44 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Please review this improvement to the `checked_cast` utility.
>> 
>> checked_cast was added by JDK-8255544 to permit silencing of certain compiler
>> warnings (such as from gcc's -Wconversion) for narrowing conversions when the
>> value is "known" to be safely convertible.  It provides debug-only runtime
>> verification that the conversion preserves the value while changing the type.
>> 
>> There has been a recent effort to apply checked_cast to eliminate -Wconversion
>> warnings, with the eventual goal of turning on such warnings by default - see
>> JDK-8135181.
>> 
>> The existing implementation checks that the value is unchanged by a round-trip
>> conversion, and has no restrictions on the arguments.  There are several
>> problems with this.
>> 
>> (1) There are some cases where conversion of an integral value to a different
>> integral type may pass the check, even though the value isn't in the range of
>> the destination type.
>> 
>> (2) Floating point to integral conversions are often intended to discard the
>> fractional part.  But that won't pass the round-trip conversion test, making
>> checked_cast mostly useless for such conversions.
>> 
>> (3) Integral to floating point conversions are often intended to be
>> indifferent to loss of precision.  But again, that won't pass the round-trip
>> conversion test, making checked_cast mostly useless for such conversions.
>> 
>> This change to checked_cast supports integral to integral conversions, but not
>> conversions involving floating point types.  The intent is that we'll use
>> "-Wconversion -Wno-float-conversion" instead of -Wconversion alone.  If/when
>> we later want to enable -Wfloat-conversion, we can either extend checked_cast
>> for that purpose, or probably better, add new functions tailored for the
>> various use-cases.
>> 
>> It also supports enum to integral conversions, mostly for compatibility with
>> old code that uses class-scoped enums instead of class-scoped static const
>> integral members, to work around ancient broken compilers.  We still have a
>> lot of such code.
>> 
>> This new checked_cast ensures (in debugging builds) that the value being
>> converted is in the range of the destination type.  It does so while avoiding
>> tautological comparisons, as some versions of some compilers may warn about
>> such.  Note that this means it can also be used to suppress -Wsign-conversion
>> warnings (which are not included in -Wconversion when compiling C++), which we
>> might explore enabling in the future.
>> 
>> It also verifi...
>
> src/hotspot/share/compiler/oopMap.hpp line 115:
> 
>> 113:     stream->write_int(value());
>> 114:     if(is_callee_saved() || is_derived_oop()) {
>> 115:       stream->write_int(content_reg()->value());
> 
> Suggestion:
> 
>       stream->write_int(checked_cast<juint>(content_reg()->value()));

Again here, just removing existing unnecessary cast.  Dealing with any further mismatch is out of scope for this PR.

> src/hotspot/share/gc/g1/g1CodeRootSet.cpp line 192:
> 
>> 190:     // we would grow again quickly.
>> 191:     const float WantedLoadFactor = 0.5;
>> 192:     assert((current_size / WantedLoadFactor) <= SIZE_MAX, "table overflow");
> 
> Surprisingly, this might not work.  See https://bugs.openjdk.org/browse/JDK-8287052.

It looks like for clang we should use -Wimplicit-int-conversion instead of
gcc's "-Wconversion -fno-float-conversion". clang seems to have a much richer
set of warning controls in this area than does gcc.

It looks like for clang we should use -Wimplicit-int-conversion instead of
gcc's "-Wconversion -fno-float-conversion". clang seems to have a much richer
set of warning controls in this area than does gcc.

That way we don't implicitly get -Wimplicit-int-float-conversion, which is
what is triggering the warning mentioned in JDK-8287052.  In this case the
loss of precision leading to that warning does not seem important.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1360021515
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1360020983