On Fri, 30 Jun 2023 01:52:40 GMT, David Holmes <dhol...@openjdk.org> 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 other action (but after lvalue-to-rvalue
conversion, if applicable), the operand undergoes [integral
promotion](https://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion).
If an operand has array or function type,
[array-to-pointer](https://en.cppreference.com/w/cpp/language/implicit_conversion#array_to_pointer_conversion)
and
[function-to-pointer](https://en.cppreference.com/w/cpp/language/implicit_conversion#function_to_pointer)
conversions are applied.
For the binary operators (except shifts), if the promoted operands have
different types, [usual arithmetic
conversions](https://en.cppreference.com/w/cpp/language/usual_arithmetic_conversions)
are applied.
char, signed char, unsigned char, short and unsigned short can be converted to
int if their respective entire value range can be held by the type int, or
unsigned int otherwise;
> 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?
Same - the ~ operator promotes to int.
> 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.
static jfieldID to_instance_jfieldID(Klass* k, int offset) {
We encode the offset as an int so to get the offset back again, it should be an
int.
I worked out the calculation once but I could just check_cast<int> the 1 jvmti
caller instead, and save the 7 jni callers for another pass.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247783660
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247833600
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247784556
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247833066
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247784903
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247825020
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247826857
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247788982
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247793928
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247802771
