Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Mon, 3 Jul 2023 01:20:08 GMT, David Holmes wrote: >> JvmtiRawMonitor _recursions is an int. Maybe it shouldn't be. You could >> file an RFE to change that if it's wrong. >> >> >> volatile int _recursions; // recursion count, 0 for first entry > > Sorry, yes was looking at the wrong `_recursions`. `int` is fine here, `intx` > is odd as the max expected recursions should not depend on 32-bit versus > 64-bit. I can't imagine why recursions would be 64 bit in the ObjectMonitor code, that does seem excessive. - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1253031977
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore 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: > > David's suggestions. Thanks David for the review. I do change the types as low in the call stack as practical with these changes. I think your comment above disappeared because the last change reverted that code. - PR Comment: https://git.openjdk.org/jdk/pull/14710#issuecomment-1621658684
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On 3/07/2023 10:55 am, David Holmes wrote: On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore 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: David's suggestions. src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 92: 90: result &= small_offset_mask; // cut off the hash bits 91: } 92: return result; Doesn't this trigger a warning as we go from unsigned to signed? Weird: I was sure I deleted this comment. Please ignore. I can't even find it in the PR so responding via email. David - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1249960932
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Fri, 30 Jun 2023 12:49:50 GMT, Coleen Phillimore wrote: >> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 385: >> >>> 383: OrderAccess::fence(); >>> 384: >>> 385: int save = _recursions; >> >> `_recursions` is `intx` > > JvmtiRawMonitor _recursions is an int. Maybe it shouldn't be. You could > file an RFE to change that if it's wrong. > > > volatile int _recursions; // recursion count, 0 for first entry Sorry, yes was looking at the wrong `_recursions`. `int` is fine here, `intx` is odd as the max expected recursions should not depend on 32-bit versus 64-bit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1249983979
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore 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: > > David's suggestions. As a general rule I'd prefer to see the lowest-level functions use the types of the actual thing they are exposing - so for anything CP related we return u1/u2/u4 - and then have higher-level code do any convenience conversions. But I realize this is tricky to deal with and there are alternatives and trade-offs in where these warnings get silenced. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1510139930
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore 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: > > David's suggestions. src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 92: > 90: result &= small_offset_mask; // cut off the hash bits > 91: } > 92: return result; Doesn't this trigger a warning as we go from unsigned to signed? - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1249960932
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Fri, 30 Jun 2023 02:18:14 GMT, David Holmes wrote: >> 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. @dholmes-ora The reason that these changes are broken up like this is because there are a huge amount of -Wconversion warnings and to do it right, we need to examine where the warnings are to see if there are bugs, and to correct the code to either use the right types or allow the cast with an assertion check if necessary. The changes are not designed to be completely minimal but balanced so that people can review them and that changes that aren't necessary aren't made. We pass around 'int' as a parameter type for values that are u2, for example. It really matters most when we're storing that value or using it in a context that requires the narrow type. If we're using it as an index which most constant pool and constant pool cache indices are used as, we can keep it as an int. A u2 parameter is promoted to int so it's the same thing. I hope I've answered your questions about these specific types in this change. - PR Comment: https://git.openjdk.org/jdk/pull/14710#issuecomment-1614635744
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 02:11:11 GMT, David Holmes wrote: >> I had to change these two lines because BytecodeStream::get_index_u2 returns >> an int, so got the warning and this didn't need to be declared with u2. >> get_index_u2() could be fixed to return a u2 but I didn't want to go that >> far as no casts were involved in this change. > > I think this change looks "wrong" - the indices are supposed to be u2's, if > the function returns an int that seems an error. Fixing these functions will have fallout further down in the code, so I didn't want to do this. These functions are used in a lot of places in the code where int is used for convenience. int is a convenient type. This didn't require a cast because the function klass_at_noresolve()'s parameter is an int. This isn't wrong. - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247802199
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Fri, 30 Jun 2023 01:52:40 GMT, David Holmes wrote:
>> 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.
>
> 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?
The code_length attribute is defined in the class file specification as u4, but
further specifies that:
code_length
The value of the code_length item gives the number of bytes in the code array
for this method.
The value of code_length must be greater than zero (as the code array must not
be empty) and less than 65536.
So we store code_length as a u2 to save space in the ConstMethod. That's the
size of the field _code_size. There are checks in set_code_size and in the
ClassFileParser to verify this:
guarantee_property(code_length > 0 && code_length <= MAX_CODE_SIZE,
"Invalid method Code length %u in class file %s",
code_length, CHECK_NULL);
And further when assigning code_size:
void set_code_size(int size) {
assert(max_method_code_size < (1 << 16),
"u2 is too small to hold method code size in general");
assert(0 <= size && size <= max_method_code_size, "invalid code size");
_code_size = (u2)size;
}
At one point, there was discussion of expanding the code_size to u4. If that
decision is made, we should change these types. I made the return type match
the field type because it's copied when we create a new method. I did that to
avoid a cast.
> 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?
see above.
> 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?
Yes.
> 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?
If the code above explicitly checks for the value of the int, then I don't use
checked_cast<>. The explicit checks are more descriptive of the expectations
and that assert would be better than the checked_cast<> assert which is
somewhat generic.
> 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?
I could change that to u4. It's the same thing. It just can't be size_t. I
really don't know why the C++ compiler gives this a Wconversion warning and not
the lines above it.
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp:402:12: warning:
conversion from 'long unsigned int' to 'u4' {aka 'unsigned int'} may change
value [-Wconversion]
402 | length += sizeof(u2) * num_bootstrap_arguments; //
bootstrap_arguments[num_bootstrap_arguments]
| ~~~^~~
> 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`?
It's the same type. Since we're not already using a jlong in this function, I
used the preferable uint64_t. I can make it jlong if you prefer that.
> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 385:
>
>> 383: OrderAccess::fence();
>> 384:
>> 385: int save = _recursions;
>
> `_recursions` is `intx`
JvmtiRawMonitor _recursions is an int. Maybe it shouldn't be. You could file
an RFE to change that if it's wrong.
volatile int _recursions; // recursion count, 0 for first entry
> 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?
Yes the ~ operator promotes to int.
https://en.cppreference.com/w/cpp/language/operator_arithmetic
Conversions
If the operand passed to an arithmetic operator is integral or unscoped
enumeration type, then before any
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore 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. The task is NOT to just silence the conversion warnings but to correct the types to be more precise to not get conversion warnings. That's what this change does. I think I've answered your questions about the types chosen. - PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1507060957
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
> 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: David's suggestions. - Changes: - all: https://git.openjdk.org/jdk/pull/14710/files - new: https://git.openjdk.org/jdk/pull/14710/files/42781421..d07a212c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14710=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14710=02-03 Stats: 6 lines in 4 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 19:44:58 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/prims/methodComparator.cpp line 79:
>>
>>> 77: case Bytecodes::_instanceof : {
>>> 78: int cpi_old = s_old->get_index_u2();
>>> 79: int cpi_new = s_new->get_index_u2();
>>
>> These constant pool accessors like `klass_at_noresolve` currently take in
>> `int which` but I think it's worth looking at if this is necessary. Constant
>> pool indices and constant pool cache indices seem to both be u2 so it might
>> be a better option to change the arguments to u2 here to avoid the need to
>> cast.
>
> I had to change these two lines because BytecodeStream::get_index_u2 returns
> an int, so got the warning and this didn't need to be declared with u2.
> get_index_u2() could be fixed to return a u2 but I didn't want to go that far
> as no casts were involved in this change.
I think this change looks "wrong" - the indices are supposed to be u2's, if the
function returns an int that seems an error.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247334013
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore 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
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 19:51:43 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2195:
>>
>>> 2193: case Bytecodes::_ldc:
>>> 2194: {
>>> 2195: u1 cp_index = *(bcp + 1);
>>
>> Constant pool indices are usually u2, why does this need to be a u1?
>
> This could be a u2 to avoid confusion. Since it's ldc, the cp_index in the
> ldc bytecode is only a u1 but this didn't get a Wconversion error so I should
> probably keep it as int.
> Edit: the bcp offset fetched is a u1 (byte) size, but we assign cp_index into
> new_index below so cp_index needs to be smaller than new_index. That's why I
> changed it. Making it u1 is more precise and doesn't have warnings.
I agree - using u1 to match the spec is a good thing here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247322728
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore 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. Thanks for your comments Matias. Some of the changes that I didn't make were because Wconversion didn't complain and where I could avoid casting. I think more precise types for constant pool indices and cpCache indices might be more pervasive than we want and might cause more warnings down the line. So I avoided them. - PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1505961331
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:23:44 GMT, Matias Saavedra Silva
wrote:
>> 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.
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2195:
>
>> 2193: case Bytecodes::_ldc:
>> 2194: {
>> 2195: u1 cp_index = *(bcp + 1);
>
> Constant pool indices are usually u2, why does this need to be a u1?
This could be a u2 to avoid confusion. Since it's ldc, the cp_index in the ldc
bytecode is only a u1 but this didn't get a Wconversion error so I should
probably keep it as int.
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2268:
>
>> 2266: {
>> 2267: address p = bcp + 1;
>> 2268: int cp_index = Bytes::get_Java_u2(p);
>
> Should this field also be a u2?
It could be a u2, but since find_new_index's parameter is int and didn't need a
cast further down, I didn't change it to one.
> src/hotspot/share/prims/methodComparator.cpp line 79:
>
>> 77: case Bytecodes::_instanceof : {
>> 78: int cpi_old = s_old->get_index_u2();
>> 79: int cpi_new = s_new->get_index_u2();
>
> These constant pool accessors like `klass_at_noresolve` currently take in
> `int which` but I think it's worth looking at if this is necessary. Constant
> pool indices and constant pool cache indices seem to both be u2 so it might
> be a better option to change the arguments to u2 here to avoid the need to
> cast.
I had to change these two lines because BytecodeStream::get_index_u2 returns an
int, so got the warning and this didn't need to be declared with u2.
get_index_u2() could be fixed to return a u2 but I didn't want to go that far
as no casts were involved in this change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247101683
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247069649
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247053822
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore 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.
Looks good, thanks for this change! I listed a few considerations below, but if
you don't think they are necessary, what you have is just fine.
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2195:
> 2193: case Bytecodes::_ldc:
> 2194: {
> 2195: u1 cp_index = *(bcp + 1);
Constant pool indices are usually u2, why does this need to be a u1?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2268:
> 2266: {
> 2267: address p = bcp + 1;
> 2268: int cp_index = Bytes::get_Java_u2(p);
Should this field also be a u2?
src/hotspot/share/prims/methodComparator.cpp line 79:
> 77: case Bytecodes::_instanceof : {
> 78: int cpi_old = s_old->get_index_u2();
> 79: int cpi_new = s_new->get_index_u2();
These constant pool accessors like `klass_at_noresolve` currently take in `int
which` but I think it's worth looking at if this is necessary. Constant pool
indices and constant pool cache indices seem to both be u2 so it might be a
better option to change the arguments to u2 here to avoid the need to cast.
-
Marked as reviewed by matsaave (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1505776921
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246928389
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246927933
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246934498
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/14710/files - new: https://git.openjdk.org/jdk/pull/14710/files/c77556da..42781421 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14710=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14710=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v2]
On Thu, 29 Jun 2023 14:34:06 GMT, Frederic Parain wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fred's comments.
>
> src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 399:
>
>> 397: int length = sizeof(u2); // num_bootstrap_methods
>> 398: for (int n = 0; n < num_bootstrap_methods; n++) {
>> 399: int num_bootstrap_arguments = cpool()->operand_argument_count_at(n);
>
> operand_arguments_count_at() returns an u2, I think it would make more sense
> to put the cast line 402 where num_bootstrap_arguments is used.
I fixed both things you noticed. Thank you!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246923278
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v2]
> 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: Fred's comments. - Changes: - all: https://git.openjdk.org/jdk/pull/14710/files - new: https://git.openjdk.org/jdk/pull/14710/files/325cd6e9..c77556da Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14710=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14710=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code
On Thu, 29 Jun 2023 12:17:23 GMT, Coleen Phillimore 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.
Marked as reviewed by fparain (Reviewer).
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 399:
> 397: int length = sizeof(u2); // num_bootstrap_methods
> 398: for (int n = 0; n < num_bootstrap_methods; n++) {
> 399: int num_bootstrap_arguments = cpool()->operand_argument_count_at(n);
operand_arguments_count_at() returns an u2, I think it would make more sense to
put the cast line 402 where num_bootstrap_arguments is used.
-
PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1505437059
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246709628
RFR: 8311077: Fix -Wconversion warnings in jvmti code.
Please review change for mostly fixing return types in the constant pool and metadata to fix -Wconversion warnings in JVMTI code. Tested with tier1-4. - Commit messages: - Fix -Wconversion warnings in jvmti code. Changes: https://git.openjdk.org/jdk/pull/14710/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14710=00 Issue: https://bugs.openjdk.org/browse/JDK-8311077 Stats: 100 lines in 16 files changed: 4 ins; 2 del; 94 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
