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

[Dean Long]

On Mon, 2 Oct 2023 03:10:29 GMT, Kim Barrett <kbarrett 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 verifies a runtime check is needed, producing a compile-time error if
> not.  Unnecessary checked_cast...

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(checked_cast<int>(content_reg()->value()));

int --> juint, we need checked_cast<juint> here because of -Wsign-conversion

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()));

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.

src/hotspot/share/gc/parallel/mutableNUMASpace.cpp line 712:

> 710:   os::page_info page_expected, page_found;
> 711:   page_expected.size = page_size;
> 712:   page_expected.lgrp_id = lgrp_id();

Suggestion:

  page_expected.lgrp_id = checked_cast<int>(lgrp_id());

uint --> int
Or maybe fix the types to be the same.

src/hotspot/share/utilities/align.hpp line 75:

> 73: template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
> 74: constexpr T align_up(T size, A alignment) {
> 75:   T adjusted = checked_cast<T, true>(size + alignment_mask(alignment));

I guess the checked_cast is needed for sizeof(T) < sizeof(int), but there is still a possible signed overflow UB here.

src/hotspot/share/utilities/checkedCast.hpp line 55:

> 53:   static constexpr bool check_range(From from) {
> 54:     To to_max = std::numeric_limits<To>::max();
> 55:     return from <= static_cast<From>(to_max);

Don't we need to check that From is big enough to contain to_max?
Are we allowed to use checked_cast for signed to unsigned widening?
    checked_cast<uint64_t, int32_t> // fail if source is negative

src/hotspot/share/utilities/checkedCast.hpp line 100:

> 98:   static_assert(permit_tautology || is_narrowing, "tautological checked_cast");
> 99:   constexpr bool operator()(From from) const {
> 100:     return !is_narrowing || (static_cast<From>(static_cast<To>(from)) == from);

OK, so we are allowing compiler-specific behavior now?  Does that mean we can get rid of the weird-looking `reinterpret_cast<TYPE&>(ures)` in `JAVA_INTEGER_OP`?

src/hotspot/share/utilities/checkedCast.hpp line 139:

> 137:     return (from >= 0);
> 138:   }
> 139: };

So instead of checked_cast<uint64_t, int32_t> or checked_cast<uint,int>, callers should check from < 0 and then do a static_cast<>?  It might be better to allow checked_cast to support non-narrowed signed to unsigned, because I think the alternative is more error-prone.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359606831
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359617905
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359626718
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359617358
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359650061
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359652902
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359656238
PR Review Comment: https://git.openjdk.org/jdk/pull/16005#discussion_r1359658456