On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Please review change for mostly fixing return types in the constant pool and
>> metadata to fix -Wconversion warnings in JVMTI code. The order of
>> preference for changes are: 1. change the types to more distinct types
>> (fields in the constant pool are u2 because that's their size in the
>> classfile), 2. add direct int casts if the value has been checked in asserts
>> above, and 3. checked_cast<> if not verified, and 4. added some
>> pointer_delta_as_ints where needed.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a comment about u1 cast to new_index for the ldc bytecode.
Sorry Coleen but this raises a lot of questions for me. I expect there are a
few ways to silence these conversion warnings but many of the proposed changes
don't look semantically right to me. I realize that we have a lot of
inconsistencies in the way we declare and use types and that the proposed
changes may be the most minimal way to fix things, but it is better to type
them correctly from the start IMO and only cast at the "boundaries" if it
requires a change to propagate too far.
src/hotspot/share/oops/constMethod.hpp line 327:
> 325:
> 326: // code size
> 327: u2 code_size() const { return _code_size; }
The `code_length` of a `Code` attribute is `u4` - why is `u2` appropriate here?
src/hotspot/share/oops/method.hpp line 257:
> 255:
> 256: // code size
> 257: u2 code_size() const { return
> constMethod()->code_size(); }
Again why is this not u4?
src/hotspot/share/oops/method.hpp line 269:
> 267: // return original max stack size for method verification
> 268: u2 verifier_max_stack() const { return
> constMethod()->max_stack(); }
> 269: int max_stack() const { return
> constMethod()->max_stack() + extra_stack_entries(); }
So this has to be greater than a `u2` because of the extra entries?
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 310:
> 308: write_attribute_name_index("MethodParameters");
> 309: write_u4(size);
> 310: write_u1((u1)length);
What is the rule for using a plain cast versus a checked_cast?
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 402:
> 400: length += sizeof(u2); // bootstrap_method_ref
> 401: length += sizeof(u2); // num_bootstrap_arguments
> 402: length += (int)sizeof(u2) * num_bootstrap_arguments; //
> bootstrap_arguments[num_bootstrap_arguments]
Not clear why the int cast is used given length is a u4?
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 83:
> 81: bool
> 82: JvmtiRawMonitor::is_valid() {
> 83: uint64_t value = 0;
Why `uint64_t` here when `JvmtiEnvBase::is_valid` uses `jlong`?
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 385:
> 383: OrderAccess::fence();
> 384:
> 385: int save = _recursions;
`_recursions` is `intx`
src/hotspot/share/prims/jvmtiTrace.cpp line 205:
> 203: _trace_flags[i] |= bits;
> 204: } else {
> 205: _trace_flags[i] &= (jbyte)~bits;
Looks strange that only this use of bits needs the cast?
src/hotspot/share/prims/jvmtiTrace.cpp line 234:
> 232: _event_trace_flags[i] |= bits;
> 233: } else {
> 234: _event_trace_flags[i] &= (jbyte)~bits;
Looks strange that only this use of bits needs the cast?
src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 87:
> 85: return ((as_uint & checked_mask_in_place) != 0);
> 86: }
> 87: static int raw_instance_offset(jfieldID id) {
This doesn't look right as we've gone from returning a 64-bit value to a 32-bit
value. If it is meant to be only 32-bit I expect the callers need some
adjustment too.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1506346034
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247323647
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247326196
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247326799
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247328384
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247329307
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247332160
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247332571
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247333138
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247333184
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247334731
