RFR: 8309685: Fix -Wconversion warnings in assembler and register code
Coleen Phillimore
coleenp at openjdk.org
Tue Jun 13 14:24:02 UTC 2023
On Tue, 13 Jun 2023 13:31:50 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> 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?
>
> The downside of this is it hides potentially real type mismatches from -Wconversion, once (if) that becomes
> part of normal operations.
Thanks for the suggested change. The sign extension for integral promotion was a problem. The second emit_int8 will still assert for the callers (there are still some) that pass a type that gets sign extended to int to emit_in8. I can see how many are left once the first function is added. Maybe it's possible to fix them all. We'll see.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14396#discussion_r1228203686
More information about the hotspot-dev
mailing list