Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Thu, 29 Jun 2023 18:40:50 GMT, Alex Menkov wrote:
>> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier
>> should return STATE_WAITING)
>> New test tests GetThreadState for different thread states.
>> The test detected a bug in the implementation, new issue is created:
>> JDK-8310584
>> Corresponding testcases are commented now in the test, fix for JDK-8310584
>> will uncomment them
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> updated comment
Thank you for the test. It is nice to have it.
It looks good.
Thanks,
Serguei
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 112:
> 110: LOG(" ERROR: all expected 'weak' bits are set\n");
> 111: }
> 112: }
Nit: I guess, error message at line 110 has to be:
`ERROR: not all expected 'weak' bits are set`
Also, it looks like the check 3a is not really needed as the 3b should cover it.
But I leave it up to you, if you think check 3a gives some convenience.
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14622#pullrequestreview-1506557170
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1247465477
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: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
On Thu, 29 Jun 2023 18:51:26 GMT, Alex Menkov wrote:
>> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
>> line 62:
>>
>>> 60: TestStatus status = new
>>> TestStatus("JVMTI_THREAD_STATE_RUNNABLE");
>>> 61: CountDownLatch ready = new CountDownLatch(1);
>>> 62: final boolean[] stopFlag = new boolean[1];
>>
>> Q: It is not clear why an array is needed instead of non-final local. The
>> same question for `waiting()` method below. I'm thinking if reverting the
>> flag to something like `needContinue` would simplify code a little bit.
>
> It's used in lambda, so must be final
> I think both "flag to stop" and "flag to continue" are clear, I prefer "flag
> to stop" as it does not require additional initialization
Okay, I see why an array is needed.
It is really minor and up to you. Just wanted to get rid of one negation. :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1247319839
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 13:05:58 GMT, Leo Korinth wrote: > My changes look like this in the diff output > ``` > } > - > ``` Thanks for showing this and other output. To me this looked like the final newline had been removed. I would have expected to see something that more obviously showed more than one blank line before hand and exactly one after. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613985636
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Thu, 29 Jun 2023 20:06:19 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into > JDK-8310829 > - [skip ci] handle pending HotSpot exception closer to site causing exception > - revert to lazy loading of VMSupport > - each exception translation failure should trigger a JVMCI event > - try harder to show nested exception during exception translation > - resolve VMSupport at bootstrap to avoid nested exception in > ExceptionTranslation::doit I am fins with idea of changes. But, please, activate GHA testing for this branch. And there is build error on Windows: c:\workspace\open\src\hotspot\share\jvmci\jvmciEnv.cpp(449): error C2220: the following warning is treated as an error c:\workspace\open\src\hotspot\share\jvmci\jvmciEnv.cpp(449): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1613932794
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Thu, 29 Jun 2023 02:13:32 GMT, David Holmes wrote: > Someone from the compiler team should review this now. @vnkozlov could you please review this or nominate someone else from the compiler team to look at it. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1613736704
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8310829 - [skip ci] handle pending HotSpot exception closer to site causing exception - revert to lazy loading of VMSupport - each exception translation failure should trigger a JVMCI event - try harder to show nested exception during exception translation - resolve VMSupport at bootstrap to avoid nested exception in ExceptionTranslation::doit - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/9236128a..e46a6a17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=04-05 Stats: 13222 lines in 537 files changed: 6305 ins; 3442 del; 3475 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
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: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
On Thu, 29 Jun 2023 14:26:10 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spurious wakeups
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 62:
>
>> 60: TestStatus status = new
>> TestStatus("JVMTI_THREAD_STATE_RUNNABLE");
>> 61: CountDownLatch ready = new CountDownLatch(1);
>> 62: final boolean[] stopFlag = new boolean[1];
>
> Q: It is not clear why an array is needed instead of non-final local. The
> same question for `waiting()` method below. I'm thinking if reverting the
> flag to something like `needContinue` would simplify code a little bit.
It's used in lambda, so must be final
I think both "flag to stop" and "flag to continue" are clear, I prefer "flag to
stop" as it does not require additional initialization
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246990707
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
On Thu, 29 Jun 2023 14:10:38 GMT, Serguei Spitsyn wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> spurious wakeups > > test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java > line 41: > >> 39: /** >> 40: * The test implements different scenarios to get desired JVMTI thread >> states. >> 41: * For each scenarios test also checks states after carrier and virtual >> threads suspend/resume > > Nit: `each scenarios` => `each scenario` Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246974900
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier > should return STATE_WAITING) > New test tests GetThreadState for different thread states. > The test detected a bug in the implementation, new issue is created: > JDK-8310584 > Corresponding testcases are commented now in the test, fix for JDK-8310584 > will uncomment them Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: updated comment - Changes: - all: https://git.openjdk.org/jdk/pull/14622/files - new: https://git.openjdk.org/jdk/pull/14622/files/6c517cef..8918411b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14622=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14622=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14622.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14622/head:pull/14622 PR: https://git.openjdk.org/jdk/pull/14622
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: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:08:15 GMT, Kevin Walls wrote: > Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14714#pullrequestreview-1505775865
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
Withdrawn: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14698
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This was not liked, I will close it. I will possibly do it under another PR for the GC code. Thanks for reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613526703
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
Re: RFR: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:22:29 GMT, Alan Bateman wrote: > Looks fine, I assume you'll bump the copyright date before integrating. Will do. - PR Comment: https://git.openjdk.org/jdk/pull/14714#issuecomment-1613500142
Re: RFR: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:08:15 GMT, Kevin Walls wrote: > Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. Looks fine, I assume you'll bump the copyright date before integrating. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14714#pullrequestreview-1505676832 PR Review: https://git.openjdk.org/jdk/pull/14714#pullrequestreview-1505677062
RFR: 8310988: Missing @since tags in java.management.rmi
Simple doc tag addition. These are files which describe an interface that has been with us since 1.5. The files themselves were previously generated at build time, but have been in the repo since jdk15. But the API is since 1.5. The other file mentioned in the bug is not a public class and has no javadoc generated, ignoring that one. - Commit messages: - 8310988: Missing @since tags in java.management.rmi Changes: https://git.openjdk.org/jdk/pull/14714/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14714=00 Issue: https://bugs.openjdk.org/browse/JDK-8310988 Stats: 4 lines in 2 files changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14714.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14714/head:pull/14714 PR: https://git.openjdk.org/jdk/pull/14714
Re: RFR: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:08:15 GMT, Kevin Walls wrote: > Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14714#pullrequestreview-1505664161
Re: RFR: 8311000: missing @since info in jdk.management
On Thu, 29 Jun 2023 11:48:43 GMT, Kevin Walls wrote: > Simple addition of a doc tag. > > src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java > is introduced in jdk7 by > 7036199: Adding a notification to the implementation of > GarbageCollectorMXBeans Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14708#pullrequestreview-1505663793
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Per had an emacs feature to remove whitespaces at the end of the line, and gave me the vim version of that. That's a nice feature. I object to this change. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613437709
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
On Thu, 29 Jun 2023 04:41:20 GMT, Alex Menkov wrote:
>> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier
>> should return STATE_WAITING)
>> New test tests GetThreadState for different thread states.
>> The test detected a bug in the implementation, new issue is created:
>> JDK-8310584
>> Corresponding testcases are commented now in the test, fix for JDK-8310584
>> will uncomment them
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> spurious wakeups
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 62:
> 60: TestStatus status = new TestStatus("JVMTI_THREAD_STATE_RUNNABLE");
> 61: CountDownLatch ready = new CountDownLatch(1);
> 62: final boolean[] stopFlag = new boolean[1];
Q: It is not clear why an array is needed instead of non-final local. The same
question for `waiting()` method below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246698740
Integrated: 8309979: BootstrapMethods attribute is missing in class files recreated by SA
On Thu, 15 Jun 2023 15:06:54 GMT, Ashutosh Mehra wrote: > Please review this PR that extends SA to write BootstrapMethods attribute > when dumping the class files. > > Tested it by dumping the class file for java/lang/String and comparing the > BootstrapMethods attribute shown by javap for the original and the dumped > class. This pull request has now been integrated. Changeset: 05c2b6cd Author:Ashutosh Mehra Committer: Kevin Walls URL: https://git.openjdk.org/jdk/commit/05c2b6cd47c68d96dcb7b3db594a334e05c6ee36 Stats: 92 lines in 3 files changed: 84 ins; 2 del; 6 mod 8309979: BootstrapMethods attribute is missing in class files recreated by SA Reviewed-by: cjplummer, kevinw - PR: https://git.openjdk.org/jdk/pull/14495
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v3]
On Thu, 22 Jun 2023 15:37:11 GMT, Ashutosh Mehra wrote: >> Please review this PR that extends SA to write BootstrapMethods attribute >> when dumping the class files. >> >> Tested it by dumping the class file for java/lang/String and comparing the >> BootstrapMethods attribute shown by javap for the original and the dumped >> class. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment about the layout of operands array in constant pool > > Signed-off-by: Ashutosh Mehra Yes 8-) - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1613261723
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v3]
On Thu, 29 Jun 2023 14:16:16 GMT, Kevin Walls wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add comment about the layout of operands array in constant pool >> >> Signed-off-by: Ashutosh Mehra > > Yes 8-) @kevinjwalls thank you! - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1613263248
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
On Thu, 29 Jun 2023 04:41:20 GMT, Alex Menkov wrote: >> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier >> should return STATE_WAITING) >> New test tests GetThreadState for different thread states. >> The test detected a bug in the implementation, new issue is created: >> JDK-8310584 >> Corresponding testcases are commented now in the test, fix for JDK-8310584 >> will uncomment them > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > spurious wakeups test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java line 41: > 39: /** > 40: * The test implements different scenarios to get desired JVMTI thread > states. > 41: * For each scenarios test also checks states after carrier and virtual > threads suspend/resume Nit: `each scenarios` => `each scenario` - PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246677973
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:40:34 GMT, Coleen Phillimore wrote: > You could fix your emacs functions. It is a *very nice* feature of global-whitespace-cleanup-mode - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613252347
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Fri, 16 Jun 2023 18:10:58 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Yes, it is already problem listed. How did you run the tests? If you use > "make test" it should be including the problem list automatically. @plummercj @kevinjwalls can either of you sponsor it as well? - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1613247796
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:11:40 GMT, David Holmes wrote: > Neither the PR diffs nor the webrev make it easy to see exactly what is being > changed here. It appeared to me that the last empty line of each file was > being deleted, leaving no newline at the end. My changes look like this in the diff output } - Removal of the last newline would look like this: -} +} \ No newline at end of file (both with `git diff` and `git diff --unified`) I have not tested if this is also true for the generated webrevs, but I think that is precisely how they are created. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613152641
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change You could fix your emacs functions. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613106245
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:01:03 GMT, Coleen Phillimore wrote: > Why do we care about this? I care because of global-whitespace-cleanup-mode (in emacs). It helps me remove trailing whitespaces and blanklines when saving but it will not fix a file that was "dirty" when it was opened. Trailing blank lines triggers it not to clean whitespaces for me. And it does not look good. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613095390
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
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Neither the PR diffs nor the webrev make it easy to see exactly what is being changed here. It appeared to me that the last empty line of each file was being deleted, leaving no newline at the end. But to me this is too disruptive with no tangible benefit. And you need buy-in from all the different areas affected by this. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613043398
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Why do we care about this? - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613018234
RFR: 8311000: missing @since info in jdk.management
Simple addition of a doc tag. src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java is introduced in jdk7 by 7036199: Adding a notification to the implementation of GarbageCollectorMXBeans - Commit messages: - 8311000: missing @since info in jdk.management Changes: https://git.openjdk.org/jdk/pull/14708/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14708=00 Issue: https://bugs.openjdk.org/browse/JDK-8311000 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14708.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14708/head:pull/14708 PR: https://git.openjdk.org/jdk/pull/14708
Integrated: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. This pull request has now been integrated. Changeset: 98a954ee Author:Artem Semenov URL: https://git.openjdk.org/jdk/commit/98a954eebc4f97dd16cb89bd4f1122952c8482ca Stats: 20 lines in 6 files changed: 14 ins; 0 del; 6 mod 8308286: Fix clang warnings in linux code Reviewed-by: avu, djelinski - PR: https://git.openjdk.org/jdk/pull/14033
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. > > It also seems far too large and disruptive. Do you still think it is too disruptive after Erik's explanation? I could split it in more reviews, but I thought that maybe it would just make the review harder. I was hoping the two `git diff` commands I gave in my first comment in combination with the clean script would make the change seem reviewable. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612660457
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. The patch is leaving exactly one newline at the end of the file. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612588091
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This seems to run contrary to the requirement that files end in a newline, which git will complain about if the newline is missing. It also seems far too large and disruptive. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612567676
