RFR: 8309685: Fix -Wconversion warnings in assembler and register code
Kim Barrett
kbarrett at openjdk.org
Tue Jun 13 06:08:56 UTC 2023
On Fri, 9 Jun 2023 15:55:32 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
> This change widens some int arguments, narrows some int returns and adds some casts and checked_casts where the compiler was doing the implicit conversion. I maintained existing types - ie if the parameter was short, I used a cast to short. Also int is used in places that might be better served as unsigned int but I didn't fix that because it would be too large and risky. The registers encode an offset in an array, so it's safe to checked_cast<> to get their encoding. This fix is limited so that the types changed and casts added are intentional.
> See CR for counts of -Wconversion warnings this resolves.
> Tested with tier1-7, also tested with tier1 on all Oracle supported platforms.
This is smaller and less painful than I expected.
src/hotspot/share/asm/assembler.hpp line 303:
> 301: // These take full width arguments and cast down without checking. Many callers have sign extension converting up to
> 302: // full int width arguments, so this casts this away.
> 303: void emit_int8( uint32_t x1) { code_section()->emit_int8((uint8_t)x1); }
It is unfortunate that emit_int8 has that name, rather than emit_uint8.
But that's probably water under the bridge; certainly so for this change.
It is also unfortunate that integral promotion and -Wconversion fight each
other pretty hard. And implicit conversions make it easy for -Wconversion
failure cases to slip in while we don't have it enabled as part of normal
operations. Here's an alternative approach:
template<typename T, ENABLE_IF(std::is_same<T, uint8_t>::value)>
void emit_int8(T v) { ... }
template<typename T,
ENABLE_IF(std::is_integral<T>::value),
ENABLE_IF(!std::is_same<T, uint8_t>::value)>
void emit_int8(T v) { emit_int8(checked_cast<uint8_t>(v)); }
Maybe that's not so bad?
src/hotspot/share/code/vmreg.cpp line 33:
> 31: // "know" that stack0 is an integer masquerading as a pointer. For the
> 32: // sake of those clients, we preserve this interface.
> 33: VMReg VMRegImpl::stack0 = (VMReg)(intptr_t)VMRegImpl::stack_0()->value();
So enlighten me - why the two-step cast?
-------------
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14396#pullrequestreview-1476308549
PR Review Comment: https://git.openjdk.org/jdk/pull/14396#discussion_r1227569203
PR Review Comment: https://git.openjdk.org/jdk/pull/14396#discussion_r1227571684
More information about the hotspot-dev
mailing list