Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]

[Thomas Stuefe]

> MEMFLAGS, as well as its enum constants, should live in its own include. 
> 
> The constants are used throughout the code base, often without needing the 
> allocation APIs exposed through allocation.hpp.
> 
> The MEMFLAGS enum def is often needed within NMT itself, again often without 
> needing allocation.hpp.
> 
> ---
> 
> This patch moves the enum to its new file.
> 
> It fixes those `allocation.hpp` includes that where only needed to get 
> MEMFLAGS. It does not fix other includes. 
> 
> For backward compatibility, until we straightened out the dependencies (e.g., 
> fixing all places where we rely on indirect includes), I added memflags.hpp 
> to allocation.hpp.
> 
> I tested (built) on:
> - MacOS aarch64, no precompiled headers, fastdebug
> - Linux x64, no precompiled headers, fastdebug, release, fastdebug crossbuild 
> to aarch64, fastdebug minimal

Thomas Stuefe has updated the pull request incrementally with one additional 
commit since the last revision:

  Update mallocLimit.hpp

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19172/files
  - new: https://git.openjdk.org/jdk/pull/19172/files/9a27048a..42361558

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

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

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

Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis

[Julian Waters]

On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie  wrote:

>> WIP
>> 
>> This changeset contains hsdis for Windows/gcc Port. It supports both the 
>> binutils and capstone backends, though the LLVM backend is left out due to 
>> compatibility issues encountered during the build. Currently, which gcc 
>> distributions are supported is still to be clarified, as several, ranging 
>> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, 
>> the build system hack in place at the moment to compile the binutils backend 
>> on Windows is still left in place, for now.
>
> Please mark the PR as draft it is not intended for review.

@magicus?

-

PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2106619396

Re: RFR: 8332112: Update nsk.share.Log to don't be Finalizable

[David Holmes]

On Sun, 12 May 2024 21:34:41 GMT, Leonid Mesnik  wrote:

> The nsk.share.Log doing some cleanup and reporting errors in the cleanup 
> method. This method is supposed to be executed by finalizer originally. 
> However, now it is called only during shutdown hook. 
> The cleanup using Cleaner doesn't work. See 
> https://bugs.openjdk.org/browse/JDK-8330760
> 
> The cleanup() method flush stream and print summary which should be already 
> printed by complain method.
> 
> This cleanup is not necessary and printing summary usually is just disabled. 
> It is enabled if the test called 'complain' method. However, the error should 
> have been printed already in this method.
> 
> So it would be simple to remove this cleanup and reduce usage of Finalizable 
> in vmTestbase tests.
> 
> Note: The 'verboseOnErrorEnabled' is just not used.
> 
> See isVerboseOnErrorEnabled.
> 
> public boolean isVerboseOnErrorEnabled() {
> return errorsSummaryEnabled;
> }
> 
> 
> Tested with by running tests with different combinations (tier4-7) and tier1.

There seems to be very little in this PR that pertains to "finalize" so perhaps 
the JBS title etc could be updated to reflect what most of this PR is actually 
about.

> However, now it is called only during shutdown hook. 

Where does this get set up?

-

PR Review: https://git.openjdk.org/jdk/pull/19209#pullrequestreview-2051556955

Re: RFR: 8330171: Lazy W^X switch implementation

[Dean Long]

On Fri, 12 Apr 2024 14:40:05 GMT, Sergey Nazarkin  wrote:

> An alternative for preemptively switching the W^X thread mode on macOS with 
> an AArch64 CPU. This implementation triggers the switch in response to the 
> SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this 
> approach, it is now feasible to eliminate all WX guards and avoid potentially 
> costly operations. However, no significant improvement or degradation in 
> performance has been observed.  Additionally, considering the issue with 
> AsyncGetCallTrace, the patched JVM has been successfully operated with 
> [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and 
> [async-profiler](https://github.com/async-profiler/async-profiler). 
> 
> Additional testing:
> - [x] MacOS AArch64 server fastdebug *gtets*
> - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4*
> - [ ] Benchmarking
> 
> @apangin and @parttimenerd could you please check the patch on your 
> scenarios??

I think there is a sweet-spot middle-ground between the two extremes: 
full-lazy, ideal for performance, and fine-grained execute-by-default, ideal 
for security.  I don't think we should change to full-lazy and remove all the 
guard rails at this time.  I am investigating execute-by-default, and it looks 
promising.

-

Changes requested by dlong (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18762#pullrequestreview-2051465621

RFR: 8332112: Update nsk.share.Log to don't be Finalizable

[Leonid Mesnik]

The nsk.share.Log doing some cleanup and reporting errors in the cleanup 
method. This method is supposed to be executed by finalizer originally. 
However, now it is called only during shutdown hook. 
The cleanup using Cleaner doesn't work. See 
https://bugs.openjdk.org/browse/JDK-8330760

The cleanup method flush stream and print summary which should be already 
printed by complain method.

This cleanup is not necessary and printing summary usually is just disabled. It 
is enabled if the test called 'complain' method. However, the error should have 
been printed already in this method.

Note: The 'verboseOnErrorEnabled' is just not used.

See isVerboseOnErrorEnabled.
public boolean isVerboseOnErrorEnabled() {
- return errorsSummaryEnabled;
- }

-

Commit messages:
 - revertdc removal
 - 8332112: Update nsk.share.Log to don't be Finalizable

Changes: https://git.openjdk.org/jdk/pull/19209/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19209=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332112
  Stats: 141 lines in 30 files changed: 2 ins; 131 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/19209.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19209/head:pull/19209

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

RFR: 8332098: Add missing `@since` tags to `jdk.jdi`

[Nizar Benalla]

Please review this simple change where I add `@since` tags to the package-info 
file of the following packages
```com.sun.jdi
com.sun.jdi.connect
com.sun.jdi.connect.spi
com.sun.jdi.event
com.sun.jdi.request

I used the unix `grep` command to find the oldest `@since` in each package and 
used that value, as it's hard to get the source code of JDK 1-5
TIA

-

Commit messages:
 - Add `@since` tags to package-info.java

Changes: https://git.openjdk.org/jdk/pull/19200/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19200=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332098
  Stats: 15 lines in 5 files changed: 10 ins; 0 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/19200.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19200/head:pull/19200

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

Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v2]

[Sebastian Lövdahl]

On Mon, 6 May 2024 18:31:06 GMT, Larry Cable  wrote:

>> Sebastian Lövdahl has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Reworked attach logic
>
> On 5/6/24 10:35 AM, Sebastian Lövdahl wrote:
>>
>> I pushed an updated attempt at this now with d3e26a0 
>> .
>>  
>> Local testing and |make test 
>> TEST="jtreg:test/hotspot/jtreg/containers"| + |make test 
>> TEST="jtreg:test/hotspot/jtreg/serviceability"| indicate that all the 
>> known use-cases work.
>>
>> Still eager to see what you come up with @larry-cable 
>> .
>>  
>> |createAttachFile| could still be improved for example. And I would 
>> also be interested in looking into writing some test for the elevated 
>> privileges case.
>>
>> —
>> Reply to this email directly, view it on GitHub 
>> ,
>>  
>> or unsubscribe 
>> .
>> You are receiving this because you were mentioned.Message ID: 
>> ***@***.***>
>>
> 
> 
>   /*
> diff --git 
> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> index 81d4fd259ed..c148dbd61b7 100644
> --- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> +++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> @@ -34,6 +34,7 @@
>   import java.nio.file.Path;
>   import java.nio.file.Paths;
>   import java.nio.file.Files;
> +import java.util.Optional;
> 
>   import static java.nio.charset.StandardCharsets.UTF_8;
> 
> @@ -46,8 +47,28 @@ public class VirtualMachineImpl extends 
> HotSpotVirtualMachine {
>   // location is the same for all processes, otherwise the tools
>   // will not be able to find all Hotspot processes.
>   // Any changes to this needs to be synchronized with HotSpot.
> -    private static final String tmpdir = "/tmp";
> +    private static final Path TMPD...

Thanks for all the deep thinking you're doing here @larry-cable, appreciated. 
And sorry for the delay in my response, I'll try to get more time devoted to 
this during the coming week.

> I did some thinking on this issue over the weekend and came up with an
> idea that *may* improve the probability of an attach succeeding in the
> case that the target has elevated privileges and the jcmd is not in the
> same mnt namespace as the target JVM.

I haven't fully digested the patches you have provided yet, but one question 
this far. In these cases, is it not a requirement that `jcmd` is run as `root`? 
So even if the target process is run with elevated privileges, attaching would 
always work. Or is there some way to attach from host to container with a 
non-`root` user that I'm missing?

One thing I would like to eventually achieve is to have 
[JDK-8226919](https://bugs.openjdk.org/browse/JDK-8226919) and this fix 
backported to the current LTS releases. Would it make more sense to fix 
[JDK-8327114](https://bugs.openjdk.org/browse/JDK-8327114) with as few changes 
as possible, and use that for backports, and do more extensive improvements in 
a separate follow-up that don't need backporting?

-

PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2106339738

Re: RFR: 8332042: Move MEMFLAGS to its own include file

[Afshin Zafari]

On Fri, 10 May 2024 09:06:08 GMT, Thomas Stuefe  wrote:

> MEMFLAGS, as well as its enum constants, should live in its own include. 
> 
> The constants are used throughout the code base, often without needing the 
> allocation APIs exposed through allocation.hpp.
> 
> The MEMFLAGS enum def is often needed within NMT itself, again often without 
> needing allocation.hpp.
> 
> ---
> 
> This patch moves the enum to its new file.
> 
> It fixes those `allocation.hpp` includes that where only needed to get 
> MEMFLAGS. It does not fix other includes. 
> 
> For backward compatibility, until we straightened out the dependencies (e.g., 
> fixing all places where we rely on indirect includes), I added memflags.hpp 
> to allocation.hpp.
> 
> I tested (built) on:
> - MacOS aarch64, no precompiled headers, fastdebug
> - Linux x64, no precompiled headers, fastdebug, release, fastdebug crossbuild 
> to aarch64, fastdebug minimal

src/hotspot/share/services/mallocLimit.hpp line 3:

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

Maybe 2023, 2024 instead?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1597683285

Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v11]

[Lei Zaakjyu]

> follow up 8267941

Lei Zaakjyu has updated the pull request incrementally with one additional 
commit since the last revision:

  fix

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18871/files
  - new: https://git.openjdk.org/jdk/pull/18871/files/65a4bbf9..dafdc775

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18871=10
 - incr: https://webrevs.openjdk.org/?repo=jdk=18871=09-10

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

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