Re: RFR: 8331557: Serial: Refactor SerialHeap::do_collection [v5]

2024-05-10 Thread Guoxiong Li
On Wed, 8 May 2024 10:04:15 GMT, Albert Mingkun Yang  wrote:

>> It's probably easier to read the new code directly. The two classes in 
>> `serialVMOperations` serve as entrance points to invoke young/full GCs. Some 
>> previously hidden decisions are made more obvious, e.g. if a young-gc fails 
>> (or will probablly fail), fallback to full-gc.
>> 
>> Additionally, `StatRecord` is removed, because this kind of info-aggregation 
>> should be done outsite VM (by third-party tool).
>> 
>> Test: tier1-6
>
> Albert Mingkun Yang has updated the pull request with a new target base due 
> to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into s1-do-collect
>  - merge
>  - review
>  - Merge branch 'master' into s1-do-collect
>  - s1-do-collect

Looks good. One suggestion, but not necessary.

src/hotspot/share/gc/serial/serialHeap.cpp line 656:

> 654:   do_full_collection_no_gc_locker(clear_soft_refs);
> 655: }
> 656: 

Maybe the method `do_young_gc` can be renamed to `do_young_collection` or 
`do_young_collection_no_gc_locker` which is consistent with 
`do_full_collection` or `do_full_collection_no_gc_locker`.

-

Marked as reviewed by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/19056#pullrequestreview-2049609145
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1596463754


Re: RFR: 8331557: Serial: Refactor SerialHeap::do_collection [v2]

2024-05-06 Thread Guoxiong Li
On Mon, 6 May 2024 09:12:47 GMT, Albert Mingkun Yang  wrote:

>> src/hotspot/share/gc/serial/serialHeap.cpp line 461:
>> 
>>> 459:   if (should_verify && VerifyBeforeGC) {
>>> 460: prepare_for_verify();
>>> 461: Universe::verify("Before GC");
>> 
>> May the prefix of the verification log be better to specify the minor or 
>> full GC? Such as `Before Minor GC` here.
>
> Other `Universe::verify("` seems to not distinguish minor/major.

OK. If someone want to change all of them in the future, she/he can file 
another ticket to follow up.

>> src/hotspot/share/gc/serial/serialHeap.cpp line 463:
>> 
>>> 461: Universe::verify("Before GC");
>>> 462:   }
>>> 463:   gc_prologue(false);
>> 
>> The parameter `full` of the method `SerialHeap::gc_prologue` doesn't been 
>> used. Seems a leftover of 
>> [JDK-8323993](https://bugs.openjdk.org/browse/JDK-8323993).
>
> True; can probably fixed in a followup cleanup.

Filed https://bugs.openjdk.org/browse/JDK-8331723 to follow up.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1590915891
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1590915740


Re: RFR: 8331557: Serial: Refactor SerialHeap::do_collection [v2]

2024-05-03 Thread Guoxiong Li
On Fri, 3 May 2024 12:49:03 GMT, Albert Mingkun Yang  wrote:

>> It's probably easier to read the new code directly. The two classes in 
>> `serialVMOperations` serve as entrance points to invoke young/full GCs. Some 
>> previously hidden decisions are made more obvious, e.g. if a young-gc fails 
>> (or will probablly fail), fallback to full-gc.
>> 
>> Additionally, `StatRecord` is removed, because this kind of info-aggregation 
>> should be done outsite VM (by third-party tool).
>> 
>> Test: tier1-6
>
> Albert Mingkun Yang has updated the pull request with a new target base due 
> to a merge or a rebase. The pull request now contains one commit:
> 
>   s1-do-collect

Nice refactor.

src/hotspot/share/gc/serial/serialHeap.cpp line 442:

> 440: }
> 441: 
> 442: bool SerialHeap::do_young_gc(DefNewGeneration* young_gen, bool 
> clear_soft_refs) {

The parameter `DefNewGeneration* young_gen` is not necessary. We can use the 
field `SerialHeap::_young_gen` directly.

src/hotspot/share/gc/serial/serialHeap.cpp line 461:

> 459:   if (should_verify && VerifyBeforeGC) {
> 460: prepare_for_verify();
> 461: Universe::verify("Before GC");

May the prefix of the verification log be better to specify the minor or full 
GC? Such as `Before Minor GC` here.

src/hotspot/share/gc/serial/serialHeap.cpp line 463:

> 461: Universe::verify("Before GC");
> 462:   }
> 463:   gc_prologue(false);

The parameter `full` of the method `SerialHeap::gc_prologue` doesn't been used. 
Seems a leftover of [JDK-8323993](https://bugs.openjdk.org/browse/JDK-8323993).

src/hotspot/share/gc/serial/serialHeap.cpp line 468:

> 466:   gen->stat_record()->accumulated_time.stop();
> 467: 
> 468:   update_gc_stats(gen, full);

The method `update_gc_stats` is only used by young-gen to sample the promoted 
size. It is good to rename and simplify the related code. I filed 
https://bugs.openjdk.org/browse/JDK-8331684 to follow up.

src/hotspot/share/gc/serial/serialHeap.cpp line 660:

> 658:   }
> 659:   do_full_collection_no_gc_locker(clear_soft_refs);
> 660: }

Please note the difference between the previous `SerialHeap::do_collection` and 
`SerialHeap::collect_at_safepoint_no_gc_locker` here. The previous 
`SerialHeap::do_collection` may invoke full GC according to the method 
`SerialHeap::should_do_full_collection` even the young GC succeeded. But 
`SerialHeap::collect_at_safepoint_no_gc_locker` only invokes full GC when the 
young GC failed (because of failed promotion). Such change makes the 
`SerialHeap::should_do_full_collection` has no user. If the behaviour of the 
`SerialHeap::collect_at_safepoint_no_gc_locker` is your intention, I think it 
is good to remove `SerialHeap::should_do_full_collection`.

-

Changes requested by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/19056#pullrequestreview-2039185857
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589844934
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589860506
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589859847
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589857649
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589863014


Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs

2024-05-03 Thread Guoxiong Li
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev  wrote:

> `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC 
> phase is running, while it actually only covers the STW GCs. It would be good 
> to rename it for clarity. The freed-up name, `is_gc_active` could then be 
> repurposed to track any (concurrent or STW) GC phase running. That would be 
> useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572).
> 
> Doing this rename separately guarantees we have caught and renamed all 
> current uses.
> 
> Additional testing:
>  - [ ] Linux AArch64 server fastdebug, `all`

Looks good.

-

Marked as reviewed by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2037637061


Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]

2024-04-24 Thread Guoxiong Li
On Tue, 23 Apr 2024 17:22:41 GMT, Guoxiong Li  wrote:

>> Hi all,
>> 
>> This patch removes the class `TenuredSpace` and adjusts its usages. After 
>> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove 
>> this file and change the included header file to `space.hpp`.
>> 
>> The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
>> review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix included header file error after merging master.

Thanks for the reviews. Integrating.

-

PR Comment: https://git.openjdk.org/jdk/pull/18894#issuecomment-2074744514


Integrated: 8330155: Serial: Remove TenuredSpace

2024-04-24 Thread Guoxiong Li
On Mon, 22 Apr 2024 16:24:06 GMT, Guoxiong Li  wrote:

> Hi all,
> 
> This patch removes the class `TenuredSpace` and adjusts its usages. After 
> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove 
> this file and change the included header file to `space.hpp`.
> 
> The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
> review.
> 
> Best Regards,
> -- Guoxiong

This pull request has now been integrated.

Changeset: 2bb5cf5f
Author:Guoxiong Li 
URL:   
https://git.openjdk.org/jdk/commit/2bb5cf5f7b2cc40aca3bdd36400dc4af5723
Stats: 162 lines in 21 files changed: 11 ins; 127 del; 24 mod

8330155: Serial: Remove TenuredSpace

Reviewed-by: ayang, cjplummer, tschatzl

-

PR: https://git.openjdk.org/jdk/pull/18894


Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]

2024-04-23 Thread Guoxiong Li
On Tue, 23 Apr 2024 17:22:41 GMT, Guoxiong Li  wrote:

>> Hi all,
>> 
>> This patch removes the class `TenuredSpace` and adjusts its usages. After 
>> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove 
>> this file and change the included header file to `space.hpp`.
>> 
>> The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
>> review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix included header file error after merging master.

I merged the master branch in order to solve the file conflict and added the 
missed header file after merging. Please take a look at the newest code.

-

PR Comment: https://git.openjdk.org/jdk/pull/18894#issuecomment-2072993947


Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]

2024-04-23 Thread Guoxiong Li
> Hi all,
> 
> This patch removes the class `TenuredSpace` and adjusts its usages. After 
> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove 
> this file and change the included header file to `space.hpp`.
> 
> The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
> review.
> 
> Best Regards,
> -- Guoxiong

Guoxiong Li has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix included header file error after merging master.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18894/files
  - new: https://git.openjdk.org/jdk/pull/18894/files/0796e0b4..5478742c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18894=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18894=01-02

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/18894.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18894/head:pull/18894

PR: https://git.openjdk.org/jdk/pull/18894


Re: RFR: 8330155: Serial: Remove TenuredSpace [v2]

2024-04-23 Thread Guoxiong Li
> Hi all,
> 
> This patch removes the class `TenuredSpace` and adjusts its usages. After 
> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove 
> this file and change the included header file to `space.hpp`.
> 
> The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
> review.
> 
> Best Regards,
> -- Guoxiong

Guoxiong Li has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains three commits:

 - Remove file.
 - Merge branch 'master' into REMOVE_TENURED_SPACE
   
   # Conflicts:
   #src/hotspot/share/gc/serial/defNewGeneration.inline.hpp
 - JDK-8330155

-

Changes: https://git.openjdk.org/jdk/pull/18894/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18894=01
  Stats: 161 lines in 20 files changed: 10 ins; 127 del; 24 mod
  Patch: https://git.openjdk.org/jdk/pull/18894.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18894/head:pull/18894

PR: https://git.openjdk.org/jdk/pull/18894


RFR: 8330155: Serial: Remove TenuredSpace

2024-04-22 Thread Guoxiong Li
Hi all,

This patch removes the class `TenuredSpace` and adjusts its usages. After 
removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove this 
file and change the included header file to `space.hpp`.

The test `make test-tier1_gc` passed locally. Thanks for taking the time to 
review.

Best Regards,
-- Guoxiong

-

Commit messages:
 - JDK-8330155

Changes: https://git.openjdk.org/jdk/pull/18894/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18894=00
  Issue: https://bugs.openjdk.org/browse/JDK-8330155
  Stats: 162 lines in 21 files changed: 10 ins; 127 del; 25 mod
  Patch: https://git.openjdk.org/jdk/pull/18894.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18894/head:pull/18894

PR: https://git.openjdk.org/jdk/pull/18894


Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE

2024-03-08 Thread Guoxiong Li
On Fri, 8 Mar 2024 02:54:49 GMT, Alex Menkov  wrote:

> RecordComponent class has _attributes_count field.
> The only user of the field is JvmtiClassFileReconstituter. Incorrect value of 
> the field causes producing incorrect data for Record attribute.
> Parsing Record attribute ClassFileParser skips unknown attributes and may 
> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
> The fix updates ClassFileParser to calculate correct attributes_count.
> 
> Testing: 
> - tier1,tier2,hs-tier5-svc;
>  - redefineClasses/retransformClasses tests:
>- test/jdk/java/lang/instrument
>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 27:

> 25:  * @test
> 26:  * @bug 8315575
> 27:  * @summary test that records with invisible annotation can be 
> retfansformed

Typo `retfansformed`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1518468461


Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]

2024-03-06 Thread Guoxiong Li
On Thu, 7 Mar 2024 07:20:15 GMT, David Holmes  wrote:

> Oracle requests/requires that the Oracle copyright always be updated when a 
> file is modified.

Got it. Thanks for your explanation.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515672685


Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]

2024-03-06 Thread Guoxiong Li
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken  wrote:

>> We define the RESTARTABLE macro again and again at a lot of places in the 
>> JDK native codebase. This could be centralized to avoid repeating it again 
>> and again !
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Introduce windows jni_util_md.h

src/java.base/aix/native/libnio/ch/Pollset.c line 3:

> 1: /*
> 2:  * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * Copyright (c) 2012, 2024 SAP SE. All rights reserved.

It seems you only need to update the copyright of the company you work for.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515417217


Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase

2024-03-06 Thread Guoxiong Li
On Wed, 6 Mar 2024 09:26:25 GMT, Matthias Baesken  wrote:

> We define the RESTARTABLE macro again and again at a lot of places in the JDK 
> native codebase. This could be centralized to avoid repeating it again and 
> again !

Looks good. Nice refactor.

-

Marked as reviewed by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1919422137


Re: RFR: 8324799: Use correct extension for C++ test headers [v2]

2024-02-28 Thread Guoxiong Li
On Thu, 29 Feb 2024 00:16:28 GMT, Kim Barrett  wrote:

>> Please review this change that renames some test .h files to .hpp.  These
>> files contain C++ code and should be named accordingly.  Some of them contain
>> uses of NULL, which we change to nullptr.
>> 
>> The renamed files are:
>> 
>> test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h
>> test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h  
>> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h
>> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h
>> test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h 
>> 
>> test/lib/jdk/test/lib/jvmti/jvmti_thread.h
>> test/lib/jdk/test/lib/jvmti/jvmti_common.h
>> test/lib/native/testlib_threads.h 
>> 
>> The #include updates were performed mostly mechanically, and builds would 
>> fail
>> if there were mistakes.  The exception is test
>> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp,
>> which was added after I'd done the mechanical update, so was updated by hand.
>> 
>> The copyright updates were similarly performed mechanically. All but a 
>> handful
>> of the including files have already had their copyrights updated recently,
>> likely as part of JDK-8324681.
>> 
>> Thus, the only "interesting" changes are to the renamed files.
>> 
>> Testing: mach5 tier1
>
> Kim Barrett has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - add Oracle copyright
>  - fix busted copyright text

Looks good.

-

Marked as reviewed by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1907811813


Re: RFR: 8324799: Use correct extension for C++ test headers

2024-02-27 Thread Guoxiong Li
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett  wrote:

> Please review this change that renames some test .h files to .hpp.  These
> files contain C++ code and should be named accordingly.  Some of them contain
> uses of NULL, which we change to nullptr.
> 
> The renamed files are:
> 
> test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h
> test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h  
> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h
> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h
> test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h 
> 
> test/lib/jdk/test/lib/jvmti/jvmti_thread.h
> test/lib/jdk/test/lib/jvmti/jvmti_common.h
> test/lib/native/testlib_threads.h 
> 
> The #include updates were performed mostly mechanically, and builds would fail
> if there were mistakes.  The exception is test
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp,
> which was added after I'd done the mechanical update, so was updated by hand.
> 
> The copyright updates were similarly performed mechanically. All but a handful
> of the including files have already had their copyrights updated recently,
> likely as part of JDK-8324681.
> 
> Thus, the only "interesting" changes are to the renamed files.
> 
> Testing: mach5 tier1

So large patch. In order to easy to review, it is good to separate such large 
patch into several patches in the future.

Two trivial places need to be adjusted.

test/hotspot/jtreg/serviceability/jvmti/events/SingleStep/singlestep01/libsinglestep01.cpp
 line 28:

> 26: #include 
> 27: #include 
> 28: #include "jvmti_common.hpp"

The copyright of this file is wrong.

>  * Copyright (c) 200
>  * git 3, 2022, Oracle and/or its affiliates. All rights reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/libGetStackTraceAndRetransformTest.cpp
 line 27:

> 25: #include 
> 26: #include "jvmti.h"
> 27: #include "jvmti_common.hpp"

The oracle copyright needs to be added in this file?

>  * Copyright (c) 2023, Datadog, Inc. All rights reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

-

Changes requested by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1905259144
PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1505322535
PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1505326445


Integrated: 8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 'ps_proc.c::process_get_lwp_regs'

2023-05-16 Thread Guoxiong Li
On Thu, 11 May 2023 18:32:36 GMT, Guoxiong Li  wrote:

> Hi all,
> 
> This patch revises the code of `ps_proc.c::process_get_lwp_regs`
> to use `PTRACE_GETREGSET` first instead of `PTRACE_GETREGS`.
> The `PTRACE_GETREGS` is not present on all architectures as the man page 
> states [1].
> And if we use `PTRACE_GETREGS` first, several tests will fail at the special 
> envs,
> such as my local riscv64-linux env. Please see [the 
> issue](https://bugs.openjdk.org/browse/JDK-8307955) for more information.
> 
> And I remove the unnecessary comments and macro,
> because the `sparc` arch related code had been removed.
> 
> Thanks for the review.
> 
> Best Regards,
> -- Guoxiong
> 
> [1] https://man7.org/linux/man-pages/man2/ptrace.2.html

This pull request has now been integrated.

Changeset: 2f1c6548
Author:Guoxiong Li 
URL:   
https://git.openjdk.org/jdk/commit/2f1c65486b1e584f9c4a2eb7af2414d032a02748
Stats: 24 lines in 1 file changed: 9 ins; 13 del; 2 mod

8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 
'ps_proc.c::process_get_lwp_regs'

Reviewed-by: cjplummer, kevinw

-

PR: https://git.openjdk.org/jdk/pull/13939


Re: RFR: 8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 'ps_proc.c::process_get_lwp_regs'

2023-05-16 Thread Guoxiong Li
On Tue, 16 May 2023 05:39:05 GMT, Chris Plummer  wrote:

>> Hi all,
>> 
>> This patch revises the code of `ps_proc.c::process_get_lwp_regs`
>> to use `PTRACE_GETREGSET` first instead of `PTRACE_GETREGS`.
>> The `PTRACE_GETREGS` is not present on all architectures as the man page 
>> states [1].
>> And if we use `PTRACE_GETREGS` first, several tests will fail at the special 
>> envs,
>> such as my local riscv64-linux env. Please see [the 
>> issue](https://bugs.openjdk.org/browse/JDK-8307955) for more information.
>> 
>> And I remove the unnecessary comments and macro,
>> because the `sparc` arch related code had been removed.
>> 
>> Thanks for the review.
>> 
>> Best Regards,
>> -- Guoxiong
>> 
>> [1] https://man7.org/linux/man-pages/man2/ptrace.2.html
>
> Changes look good. I tested with our internal test system on all supported 
> platforms and did not see any issues.

@plummercj @kevinjwalls Thanks for the reviews. Integrating.

-

PR Comment: https://git.openjdk.org/jdk/pull/13939#issuecomment-1550618709


Re: RFR: 8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 'ps_proc.c::process_get_lwp_regs'

2023-05-16 Thread Guoxiong Li
On Tue, 16 May 2023 05:39:05 GMT, Chris Plummer  wrote:

>> Hi all,
>> 
>> This patch revises the code of `ps_proc.c::process_get_lwp_regs`
>> to use `PTRACE_GETREGSET` first instead of `PTRACE_GETREGS`.
>> The `PTRACE_GETREGS` is not present on all architectures as the man page 
>> states [1].
>> And if we use `PTRACE_GETREGS` first, several tests will fail at the special 
>> envs,
>> such as my local riscv64-linux env. Please see [the 
>> issue](https://bugs.openjdk.org/browse/JDK-8307955) for more information.
>> 
>> And I remove the unnecessary comments and macro,
>> because the `sparc` arch related code had been removed.
>> 
>> Thanks for the review.
>> 
>> Best Regards,
>> -- Guoxiong
>> 
>> [1] https://man7.org/linux/man-pages/man2/ptrace.2.html
>
> Changes look good. I tested with our internal test system on all supported 
> platforms and did not see any issues.

@plummercj Thanks for your test and review.

Waiting for another review.

-

PR Comment: https://git.openjdk.org/jdk/pull/13939#issuecomment-1549415870


RFR: 8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 'ps_proc.c::process_get_lwp_regs'

2023-05-11 Thread Guoxiong Li
Hi all,

This patch revises the code of `ps_proc.c::process_get_lwp_regs`
to use `PTRACE_GETREGSET` first instead of `PTRACE_GETREGS`.
The `PTRACE_GETREGS` is not present on all architectures as the man page states 
[1].
And if we use `PTRACE_GETREGS` first, several tests will fail at the special 
envs,
such as my local riscv64-linux env. Please see [the 
issue](https://bugs.openjdk.org/browse/JDK-8307955) for more information.

And I remove the unnecessary comments and macro,
because the `sparc` arch related code had been removed.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://man7.org/linux/man-pages/man2/ptrace.2.html

-

Commit messages:
 - JDK-8307955

Changes: https://git.openjdk.org/jdk/pull/13939/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=13939=00
  Issue: https://bugs.openjdk.org/browse/JDK-8307955
  Stats: 24 lines in 1 file changed: 9 ins; 13 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/13939.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13939/head:pull/13939

PR: https://git.openjdk.org/jdk/pull/13939