Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Tue, 22 Oct 2024 21:30:40 GMT, Erik Joelsson wrote: > The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large > number of individual copy rules, which can execute in any order. The > "install-file" (our copy) macro on macos includes a check for weird > attributes using `xattr` so that we can remove them. We use the switch `-s` > to make sure that xattr operates on symlinks instead of their targets. > However, if we find something, we also run `chmod` to make sure we have the > permissions to remove attributes in the first place, but the chmod command > does not have the `-h` switch to operate on the symlink instead of the > target. So if the symlink gets copied before its target, there is a chance > that we try to run chmod before the target exists, which will result in a > failure. My proposed fix is to add `-h` to the chmod command so that > everything operates on the symlink instead of the target. Changes requested by lucy (Reviewer). make/common/FileUtils.gmk line 141: > 139: $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ > 140:$(XATTR) -cs '$(call DecodeSpace, $@)'; \ > 141: fi What about running chmod only against real files? else \ $(CP) -fRP '$(call DecodeSpace, $<)' '$(call DecodeSpace, $@)'; \ if [ -n "`$(XATTR) -ls '$(call DecodeSpace, $@)'`" ]; then \ $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ $(XATTR) -cs '$(call DecodeSpace, $@)'; \ fi \ fi And maybe add an explaining comment after line 122: # The above fixup actions are only necessary if the file is actually copied. # When creating just a softlink, we rely on the softlink target being fixed when copied. - PR Review: https://git.openjdk.org/jdk/pull/21649#pullrequestreview-2387551268 PR Review Comment: https://git.openjdk.org/jdk/pull/21649#discussion_r1812046118
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering
On Mon, 21 Oct 2024 04:11:03 GMT, Jasmine Karthikeyan
wrote:
> Hi all,
> This patch adds a new pass to consolidate lowering of complex
> backend-specific code patterns, such as `MacroLogicV` and the optimization
> proposed by #21244. Moving these optimizations to backend code can simplify
> shared code, while also making it easier to develop more in-depth
> optimizations. The linked bug has an example of a new optimization this could
> enable. The new phase does GVN to de-duplicate nodes and calls nodes'
> `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid
> undoing any changes done during lowering. It also reuses the IGVN worklist to
> avoid needing to re-create the notification mechanism.
>
> In this PR only the skeleton code for the pass is added, moving `MacroLogicV`
> to this system will be done separately in a future patch. Tier 1 tests pass
> on my linux x64 machine. Feedback on this patch would be greatly appreciated!
src/hotspot/cpu/arm/c2_lowering_arm.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
Suggestion:
Node* PhaseLowering::lower_node(Node* n) {
src/hotspot/cpu/ppc/c2_lowering_ppc.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
Suggestion:
Node* PhaseLowering::lower_node(Node* n) {
src/hotspot/cpu/riscv/c2_lowering_riscv.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
Suggestion:
Node* PhaseLowering::lower_node(Node* n) {
src/hotspot/cpu/s390/c2_lowering_s390.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
Suggestion:
Node* PhaseLowering::lower_node(Node* n) {
src/hotspot/cpu/x86/c2_lowering_x86.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
Suggestion:
Node* PhaseLowering::lower_node(Node* n) {
src/hotspot/share/opto/compile.cpp line 2466:
> 2464: print_method(PHASE_BEFORE_LOWERING, 3);
> 2465:
> 2466: PhaseLowering lower(&igvn);
Any specific reason to have lowering after loop optimizations ?
Lowered nodes may change the loop body size thereby impacting unrolling
decisions.
src/hotspot/share/opto/phaseX.cpp line 2301:
> 2299: while(_igvn->_worklist.size() != 0) {
> 2300: Node* n = _igvn->_worklist.pop();
> 2301: Node* new_node = lower_node(n);
_PhaseLowring::lower_node_ may do complex transformation where by replacing a
graph pallet rooted at current node by another pallet. For each newly created
node in new pallet, it should make sure to either directly run _igvn.transform,
thereby triggering Ideal / Identity / Value sub-passed over it, OR insert the
node into _igvn.worklist for lazy processing, in latter case you are consuming
entire worklist after running over only Value transforms before existing the
lowering phase.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812113082
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812113953
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812114516
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812118894
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812119441
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812140992
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1812110851
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Tue, 15 Oct 2024 20:22:52 GMT, Magnus Ihse Bursie wrote: >> As a prerequisite for Hermetic Java, we need a statically linked `java` >> launcher. It should behave like the normal, dynamically linked `java` >> launcher, except that all JDK native libraries should be statically, not >> dynamically, linked. >> >> This patch is the first step towards this goal. It will generate a >> `static-jdk` image with a statically linked launcher. This launcher is >> missing several native libs, however, and does therefore not behave like a >> proper dynamic java. One of the reasons for this is that local symbol hiding >> in static libraries are not implemented yet, which causes symbol clashes >> when linking all static libraries together. This will be addressed in an >> upcoming patch. >> >> All changes in the `src` directory are copied from, or inspired by, changes >> made in [the hermetic-java-runtime branch in Project >> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Don't hardcode server variant I tried to build/run this on Linux as well (since I'm using parts of this PR in https://github.com/openjdk/mobile/pull/30) and I noticed that the executable doesn't start (even with --version): images/static-jdk/bin/java --version Error: Failed , because images/static-jdk/bin/java: undefined symbol: JNI_CreateJavaVM The `JNI_CreateJavaVM` symbol seems to be local only: nm images/static-jdk/bin/java |grep JNI_CreateJ 00e22430 t JNI_CreateJavaVM Hence calling it with `dlsym` won't work. And indeed, that is how it is called from `./java.base/unix/native/libjli/java_md.c ` `(LoadJavaVM)` dlsym(libjvm, "JNI_CreateJavaVM"); - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2431273631
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v50]
On Tue, 22 Oct 2024 16:19:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update copyright > - Avoid assert/endless-loop in JFR code I've published an upstream PR for the SA bug: https://github.com/openjdk/jdk/pull/21662 - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2431837874
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote:
>> This is the implementation of JEP 486: Permanently Disable the Security
>> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The
>> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the
>> main changes in the JEP and also includes an apidiff of the specification
>> changes.
>>
>> NOTE: the majority (~95%) of the changes in this PR are test updates
>> (removal/modifications) and API specification changes, the latter mostly to
>> remove `@throws SecurityException`. The remaining changes are primarily the
>> removal of the `SecurityManager`, `Policy`, `AccessController` and other
>> Security Manager API implementations. There is very little new code.
>>
>> The code changes can be broken down into roughly the following categories:
>>
>> 1. Degrading the behavior of Security Manager APIs to either throw
>> Exceptions by default or provide an execution environment that disallows
>> access to all resources by default.
>> 2. Changing hundreds of methods and constructors to no longer throw a
>> `SecurityException` if a Security Manager was enabled. They will operate as
>> they did in JDK 23 with no Security Manager enabled.
>> 3. Changing the `java` command to exit with a fatal error if a Security
>> Manager is enabled.
>> 4. Removing the hotspot native code for the privileged stack walk and the
>> inherited access control context. The remaining hotspot code and tests
>> related to the Security Manager will be removed immediately after
>> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
>> 5. Removing or modifying hundreds of tests. Many tests that tested Security
>> Manager behavior are no longer relevant and thus have been removed or
>> modified.
>>
>> There are a handful of Security Manager related tests that are failing and
>> are at the end of the `test/jdk/ProblemList.txt`,
>> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt`
>> files - these will be removed or separate bugs will be filed before
>> integrating this PR.
>>
>> Inside the JDK, we have retained calls to
>> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged`
>> for now, as these methods have been degraded to behave the same as they did
>> in JDK 23 with no Security Manager enabled. After we integrate this JEP,
>> those calls will be removed in each area (client-libs, core-libs, security,
>> etc).
>>
>> I don't expect each reviewer to review all the code changes in this JEP.
>> Rather, I advise that you only focus on the changes for the area
>> (client-libs, core-libs, net, ...
>
> Sean Mullan has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 97 commits:
>
> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
> - Change apiNote to deprecated annotation on checkAccess methods. Change
> method dedescription to "Does nothing".
> - Sanitize the class descriptions of DelegationPermission and
> ServicePermission
>by removing text that refers to granting permissions, but avoid changes
> that
>affect the API specification, such as the description and format of input
>parameters.
> - Restored methods in RMIConnection to throw SecurityExceptions again but
>with adjusted text that avoids the word "permission".
> - Add text to class description of MBeanServer stating that implementations
>may throw SecurityException if authorization doesn't allow access to
> resource.
> - Restore text about needing permissions from the desktop environment in the
>getPixelColor and createScreenCapture methods.
> - Add api note to getClassContext to use StackWalker instead and
>add DROP_METHOD_INFO option to StackWalker.
> - Change checkAccess() methods to be no-ops, rather than throwing
>SecurityException.
> - Merge
> - Merge
> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
test/jdk/java/net/httpclient/websocket/security/WSURLPermissionTest.java line
342:
> 340: throws Exception
> 341: {
> 342: action.run();
testWithNoSecurityManager was previously a sanity check, the test was focused
on permission check. Is the test still useful to keep, maybe it would be
renamed or the test method renamed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812582305
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Tue, 22 Oct 2024 21:20:59 GMT, Mandy Chung wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 97 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Change apiNote to deprecated annotation on checkAccess methods. Change >> method dedescription to "Does nothing". >> - Sanitize the class descriptions of DelegationPermission and >> ServicePermission >>by removing text that refers to granting permissions, but avoid changes >> that >>affect the API specification, such as the description and format of input >>parameters. >> - Restored methods in RMIConnection to throw SecurityExceptions again but >>with adjusted text that avoids the word "permission". >> - Add text to class description of MBeanServer stating that implementations >>may throw SecurityException if authorization doesn't allow access to >> resource. >> - Restore text about needing permissions from the desktop environment in the >>getPixelColor and createScreenCapture methods. >> - Add api note to getClassContext to use StackWalker instead and >>add DROP_METHOD_INFO option to StackWalker. >> - Change checkAccess() methods to be no-ops, rather than throwing >>SecurityException. >> - Merge >> - Merge >> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 > > test/jdk/java/lang/invoke/RevealDirectTest.java line 33: > >> 31: * @test >> 32: * @summary verify Lookup.revealDirect on a variety of input handles, >> with security manager >> 33: * @run >> main/othervm/policy=jtreg.security.policy/secure=java.lang.SecurityManager >> -ea -esa test.java.lang.invoke.RevealDirectTest > > line 36 can also be removed. > > > * $ $JAVA8X_HOME/bin/java -cp $JUNIT4_JAR:../../../.. -ea -esa > -Djava.security.manager test.java.lang.invoke.RevealDirectTest hasSM and the code that only runs when true can be deleted too. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812587449
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Mon, 21 Oct 2024 17:20:15 GMT, Sean Mullan wrote:
> > There are a couple of micro benchmarks in test/micro that fork with
> > `jvmArgsPrepend={"-Djava.security.manager=allow"})`, they will need to be
> > examined.
>
> Fixed, will be in next drop. There are a couple of other micro tests that
> test the performance of `AccessController.doPrivileged` and `getContext`
> which probably don't make sense anymore, but I've left them for now to be
> looked at later.
I checked the commit in the sandbox and it looks okay. It may be some future
maintenance in these micros may do some pruning as aren't too useful now.
-
PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2431865182
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 241: > 239: Properties props = System.getProperties(); > 240: props.setProperty(JDK_LANG_PROCESS_ALLOW_AMBIGUOUS_COMMANDS, ""); > 241: System.setSecurityManager(new SecurityMan()); I assume SecurityMan should be removed too. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812614134
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Tue, 22 Oct 2024 21:30:40 GMT, Erik Joelsson wrote: > The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large > number of individual copy rules, which can execute in any order. The > "install-file" (our copy) macro on macos includes a check for weird > attributes using `xattr` so that we can remove them. We use the switch `-s` > to make sure that xattr operates on symlinks instead of their targets. > However, if we find something, we also run `chmod` to make sure we have the > permissions to remove attributes in the first place, but the chmod command > does not have the `-h` switch to operate on the symlink instead of the > target. So if the symlink gets copied before its target, there is a chance > that we try to run chmod before the target exists, which will result in a > failure. My proposed fix is to add `-h` to the chmod command so that > everything operates on the symlink instead of the target. Marked as reviewed by ihse (Reviewer). This finally explains the odd problem you reported in https://github.com/openjdk/jdk/pull/20837#issuecomment-2327132241. That has been worrying me a bit, if there were some issue with that PR after all, but it turrned out to just be an unlikely race. *phew* - PR Review: https://git.openjdk.org/jdk/pull/21649#pullrequestreview-2389221730 PR Comment: https://git.openjdk.org/jdk/pull/21649#issuecomment-2432657995
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did The `security.cpp` looks fine to me. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2432824930
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Tue, 22 Oct 2024 21:30:40 GMT, Erik Joelsson wrote: > The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large > number of individual copy rules, which can execute in any order. The > "install-file" (our copy) macro on macos includes a check for weird > attributes using `xattr` so that we can remove them. We use the switch `-s` > to make sure that xattr operates on symlinks instead of their targets. > However, if we find something, we also run `chmod` to make sure we have the > permissions to remove attributes in the first place, but the chmod command > does not have the `-h` switch to operate on the symlink instead of the > target. So if the symlink gets copied before its target, there is a chance > that we try to run chmod before the target exists, which will result in a > failure. My proposed fix is to add `-h` to the chmod command so that > everything operates on the symlink instead of the target. > This finally explains the odd problem you reported in [#20837 > (comment)](https://github.com/openjdk/jdk/pull/20837#issuecomment-2327132241). > That has been worrying me a bit, if there were some issue with that PR after > all, but it turrned out to just be an unlikely race. _phew_ I'm not sure that error was caused by this issue. That would imply that I had strange file attributes on files in my build directory, and I don't think I do. - PR Comment: https://git.openjdk.org/jdk/pull/21649#issuecomment-2432943275
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Wed, 23 Oct 2024 07:23:44 GMT, Lutz Schmidt wrote: >> The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large >> number of individual copy rules, which can execute in any order. The >> "install-file" (our copy) macro on macos includes a check for weird >> attributes using `xattr` so that we can remove them. We use the switch `-s` >> to make sure that xattr operates on symlinks instead of their targets. >> However, if we find something, we also run `chmod` to make sure we have the >> permissions to remove attributes in the first place, but the chmod command >> does not have the `-h` switch to operate on the symlink instead of the >> target. So if the symlink gets copied before its target, there is a chance >> that we try to run chmod before the target exists, which will result in a >> failure. My proposed fix is to add `-h` to the chmod command so that >> everything operates on the symlink instead of the target. > > make/common/FileUtils.gmk line 141: > >> 139: $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ >> 140: $(XATTR) -cs '$(call DecodeSpace, $@)'; \ >> 141: fi > > What about running chmod only against real files? > > else \ > $(CP) -fRP '$(call DecodeSpace, $<)' '$(call DecodeSpace, $@)'; \ > if [ -n "`$(XATTR) -ls '$(call DecodeSpace, $@)'`" ]; then \ > $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ > $(XATTR) -cs '$(call DecodeSpace, $@)'; \ > fi \ > fi > > And maybe add an explaining comment after line 122: > > # The above fixup actions are only necessary if the file is actually copied. > # When creating just a softlink, we rely on the softlink target being fixed > when copied. The user reporting this issue actually had an attribute on the softlink itself, which I think we want to remove, so I would rather operate on the softlink. I haven't investigated how the attribute appeared on the softlink, but the purpose of this code is to clear away any unexpected attributes from files in the image. - PR Review Comment: https://git.openjdk.org/jdk/pull/21649#discussion_r1812654340
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering
On Wed, 23 Oct 2024 08:10:38 GMT, Jatin Bhateja wrote: >> Hi all, >> This patch adds a new pass to consolidate lowering of complex >> backend-specific code patterns, such as `MacroLogicV` and the optimization >> proposed by #21244. Moving these optimizations to backend code can simplify >> shared code, while also making it easier to develop more in-depth >> optimizations. The linked bug has an example of a new optimization this >> could enable. The new phase does GVN to de-duplicate nodes and calls nodes' >> `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid >> undoing any changes done during lowering. It also reuses the IGVN worklist >> to avoid needing to re-create the notification mechanism. >> >> In this PR only the skeleton code for the pass is added, moving >> `MacroLogicV` to this system will be done separately in a future patch. Tier >> 1 tests pass on my linux x64 machine. Feedback on this patch would be >> greatly appreciated! > > src/hotspot/share/opto/compile.cpp line 2466: > >> 2464: print_method(PHASE_BEFORE_LOWERING, 3); >> 2465: >> 2466: PhaseLowering lower(&igvn); > > Any specific reason to have lowering after loop optimizations ? > Lowered nodes may change the loop body size thereby impacting unrolling > decisions. Because lowering is a transformation that increases the complexity of the graph. - A `d = ExtractD(z, 4)` expanded into `x = VectorExtract(z, 2); d = ExtractD(x, 0)` increases the number of nodes by 1. - A logic cone transformed into a `MacroLogicV` introduces another kind of node that may not be recognized by other nodes. As a result, we should do this as the last step when other transformation has finished their jobs. For the concerns regarding loop body size, we still have a function in `Matcher` for that purpose. - PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1813233875
Integrated: 8342858: Make target mac-jdk-bundle fails on chmod command
On Tue, 22 Oct 2024 21:30:40 GMT, Erik Joelsson wrote: > The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large > number of individual copy rules, which can execute in any order. The > "install-file" (our copy) macro on macos includes a check for weird > attributes using `xattr` so that we can remove them. We use the switch `-s` > to make sure that xattr operates on symlinks instead of their targets. > However, if we find something, we also run `chmod` to make sure we have the > permissions to remove attributes in the first place, but the chmod command > does not have the `-h` switch to operate on the symlink instead of the > target. So if the symlink gets copied before its target, there is a chance > that we try to run chmod before the target exists, which will result in a > failure. My proposed fix is to add `-h` to the chmod command so that > everything operates on the symlink instead of the target. This pull request has now been integrated. Changeset: a522d216 Author:Erik Joelsson URL: https://git.openjdk.org/jdk/commit/a522d216b5bebbf103e5a823f0bba22cf1508883 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8342858: Make target mac-jdk-bundle fails on chmod command Reviewed-by: lucy, ihse - PR: https://git.openjdk.org/jdk/pull/21649
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering
On Wed, 23 Oct 2024 17:28:25 GMT, Quan Anh Mai wrote: >> src/hotspot/share/opto/compile.cpp line 2466: >> >>> 2464: print_method(PHASE_BEFORE_LOWERING, 3); >>> 2465: >>> 2466: PhaseLowering lower(&igvn); >> >> Any specific reason to have lowering after loop optimizations ? >> Lowered nodes may change the loop body size thereby impacting unrolling >> decisions. > > Because lowering is a transformation that increases the complexity of the > graph. > > - A `d = ExtractD(z, 4)` expanded into `x = VectorExtract(z, 2); d = > ExtractD(x, 0)` increases the number of nodes by 1. > - A logic cone transformed into a `MacroLogicV` introduces another kind of > node that may not be recognized by other nodes. > > As a result, we should do this as the last step when other transformation has > finished their jobs. For the concerns regarding loop body size, we still have > a function in `Matcher` for that purpose. Another reason is that lowering being done late allows us to have more freedom to break some invariants of the nodes, such as looking through `VectorReinterpret`. An example is this (really crafted) case: Int256Vector v; int a = v.lane(5); float b = v.reinterpretAsFloats().lane(7); This would be transformed into: vector v; vector v0 = VectorExtract(v, 1); int a = ExtractI(v0, 1); vector v1 = VectorReinterpret(v, ); vector v2 = VectorExtract(v1, 1); float b = ExtractF(v2, 3); By allowing lowering to look through `VectorReinterpret` and break the invariant of `Extract` nodes that the element types of their inputs and outputs must be the same, we can `gvn` `v1` and `v`, `v2` and `v0`. Simplify the graph: vector v; vector v0 = VectorExtract(v, 1); int a = ExtractI(v0, 1); float b = ExtractF(v0, 3); - PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1813252989
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Tue, 22 Oct 2024 21:30:40 GMT, Erik Joelsson wrote: > The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large > number of individual copy rules, which can execute in any order. The > "install-file" (our copy) macro on macos includes a check for weird > attributes using `xattr` so that we can remove them. We use the switch `-s` > to make sure that xattr operates on symlinks instead of their targets. > However, if we find something, we also run `chmod` to make sure we have the > permissions to remove attributes in the first place, but the chmod command > does not have the `-h` switch to operate on the symlink instead of the > target. So if the symlink gets copied before its target, there is a chance > that we try to run chmod before the target exists, which will result in a > failure. My proposed fix is to add `-h` to the chmod command so that > everything operates on the symlink instead of the target. LGTM. - Marked as reviewed by lucy (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21649#pullrequestreview-2388698027
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Wed, 23 Oct 2024 12:32:44 GMT, Erik Joelsson wrote: >> make/common/FileUtils.gmk line 141: >> >>> 139: $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ >>> 140: $(XATTR) -cs '$(call DecodeSpace, $@)'; \ >>> 141:fi >> >> What about running chmod only against real files? >> >> else \ >>$(CP) -fRP '$(call DecodeSpace, $<)' '$(call DecodeSpace, $@)'; \ >>if [ -n "`$(XATTR) -ls '$(call DecodeSpace, $@)'`" ]; then \ >> $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \ >> $(XATTR) -cs '$(call DecodeSpace, $@)'; \ >>fi \ >> fi >> >> And maybe add an explaining comment after line 122: >> >> # The above fixup actions are only necessary if the file is actually >> copied. >> # When creating just a softlink, we rely on the softlink target being >> fixed when copied. > > The user reporting this issue actually had an attribute on the softlink > itself, which I think we want to remove, so I would rather operate on the > softlink. I haven't investigated how the attribute appeared on the softlink, > but the purpose of this code is to clear away any unexpected attributes from > files in the image. This phenomenon is exploiting all possible facets... In that case, massaging the softlink is necessary, I agree. - PR Review Comment: https://git.openjdk.org/jdk/pull/21649#discussion_r1812762529
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 11:54:39 GMT, Alan Bateman wrote:
>> Sean Mullan has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 97 commits:
>>
>> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>> - Change apiNote to deprecated annotation on checkAccess methods. Change
>> method dedescription to "Does nothing".
>> - Sanitize the class descriptions of DelegationPermission and
>> ServicePermission
>>by removing text that refers to granting permissions, but avoid changes
>> that
>>affect the API specification, such as the description and format of input
>>parameters.
>> - Restored methods in RMIConnection to throw SecurityExceptions again but
>>with adjusted text that avoids the word "permission".
>> - Add text to class description of MBeanServer stating that implementations
>>may throw SecurityException if authorization doesn't allow access to
>> resource.
>> - Restore text about needing permissions from the desktop environment in the
>>getPixelColor and createScreenCapture methods.
>> - Add api note to getClassContext to use StackWalker instead and
>>add DROP_METHOD_INFO option to StackWalker.
>> - Change checkAccess() methods to be no-ops, rather than throwing
>>SecurityException.
>> - Merge
>> - Merge
>> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
>
> test/jdk/java/net/httpclient/websocket/security/WSURLPermissionTest.java line
> 342:
>
>> 340: throws Exception
>> 341: {
>> 342: action.run();
>
> testWithNoSecurityManager was previously a sanity check, the test was focused
> on permission check. Is the test still useful to keep, maybe it would be
> renamed or the test method renamed?
Good point. Similarly, the URLPermission[] parameter is now always unused, so
maybe I should get rid of that too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812727202
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 05:11:19 GMT, Prasanta Sadhukhan wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 97 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Change apiNote to deprecated annotation on checkAccess methods. Change >> method dedescription to "Does nothing". >> - Sanitize the class descriptions of DelegationPermission and >> ServicePermission >>by removing text that refers to granting permissions, but avoid changes >> that >>affect the API specification, such as the description and format of input >>parameters. >> - Restored methods in RMIConnection to throw SecurityExceptions again but >>with adjusted text that avoids the word "permission". >> - Add text to class description of MBeanServer stating that implementations >>may throw SecurityException if authorization doesn't allow access to >> resource. >> - Restore text about needing permissions from the desktop environment in the >>getPixelColor and createScreenCapture methods. >> - Add api note to getClassContext to use StackWalker instead and >>add DROP_METHOD_INFO option to StackWalker. >> - Change checkAccess() methods to be no-ops, rather than throwing >>SecurityException. >> - Merge >> - Merge >> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 > >> @prsadhuk Addressed review comments in the following jep486 branch commit: >> [openjdk/jdk-sandbox@d9ee496](https://github.com/openjdk/jdk-sandbox/commit/d9ee496f7349cb8beaf1e696fd430f8064baee8e) >> >> 1. test/jdk/javax/swing/JEditorPane/8080972/TestJEditor.java - Updated, >> removed redundant try-catch block >> >> 2. test/jdk/javax/swing/JPopupMenu/6691503/bug6691503.java - Repurposed >> and added back >> >> 3. test/jdk/javax/swing/JPopupMenu/6694823/bug6694823.java - Updated, >> added finally block >> >> 4. test/jdk/javax/swing/UIDefaults/6795356/TableTest.java - Added back >> >> >> Left out comments that fall into out-of-scope/clean-up for jep486. > > looks ok but awt import should have been before swing as decided, although > this again is not part of SM exercise but since you modified the tests I > thought I will say it aloud.. @prsadhuk > looks ok but awt import should have been before swing as decided, although > this again is not part of SM exercise but since you modified the tests I > thought I will say it aloud Thanks! I didn't notice it happened again with the new changes. This was probably due to IDE settings. Since this was a recent change I have updated in the latest commit. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433031866
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 Finished reviewing of accessibility (both in com.sun and jdk.javax), jdk.javax.sound and jdk.java.awt.Desktop. Looks good. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2432939145
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote:
>> This is the implementation of JEP 486: Permanently Disable the Security
>> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The
>> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the
>> main changes in the JEP and also includes an apidiff of the specification
>> changes.
>>
>> NOTE: the majority (~95%) of the changes in this PR are test updates
>> (removal/modifications) and API specification changes, the latter mostly to
>> remove `@throws SecurityException`. The remaining changes are primarily the
>> removal of the `SecurityManager`, `Policy`, `AccessController` and other
>> Security Manager API implementations. There is very little new code.
>>
>> The code changes can be broken down into roughly the following categories:
>>
>> 1. Degrading the behavior of Security Manager APIs to either throw
>> Exceptions by default or provide an execution environment that disallows
>> access to all resources by default.
>> 2. Changing hundreds of methods and constructors to no longer throw a
>> `SecurityException` if a Security Manager was enabled. They will operate as
>> they did in JDK 23 with no Security Manager enabled.
>> 3. Changing the `java` command to exit with a fatal error if a Security
>> Manager is enabled.
>> 4. Removing the hotspot native code for the privileged stack walk and the
>> inherited access control context. The remaining hotspot code and tests
>> related to the Security Manager will be removed immediately after
>> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
>> 5. Removing or modifying hundreds of tests. Many tests that tested Security
>> Manager behavior are no longer relevant and thus have been removed or
>> modified.
>>
>> There are a handful of Security Manager related tests that are failing and
>> are at the end of the `test/jdk/ProblemList.txt`,
>> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt`
>> files - these will be removed or separate bugs will be filed before
>> integrating this PR.
>>
>> Inside the JDK, we have retained calls to
>> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged`
>> for now, as these methods have been degraded to behave the same as they did
>> in JDK 23 with no Security Manager enabled. After we integrate this JEP,
>> those calls will be removed in each area (client-libs, core-libs, security,
>> etc).
>>
>> I don't expect each reviewer to review all the code changes in this JEP.
>> Rather, I advise that you only focus on the changes for the area
>> (client-libs, core-libs, net, ...
>
> Sean Mullan has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 97 commits:
>
> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
> - Change apiNote to deprecated annotation on checkAccess methods. Change
> method dedescription to "Does nothing".
> - Sanitize the class descriptions of DelegationPermission and
> ServicePermission
>by removing text that refers to granting permissions, but avoid changes
> that
>affect the API specification, such as the description and format of input
>parameters.
> - Restored methods in RMIConnection to throw SecurityExceptions again but
>with adjusted text that avoids the word "permission".
> - Add text to class description of MBeanServer stating that implementations
>may throw SecurityException if authorization doesn't allow access to
> resource.
> - Restore text about needing permissions from the desktop environment in the
>getPixelColor and createScreenCapture methods.
> - Add api note to getClassContext to use StackWalker instead and
>add DROP_METHOD_INFO option to StackWalker.
> - Change checkAccess() methods to be no-ops, rather than throwing
>SecurityException.
> - Merge
> - Merge
> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
src/hotspot/share/prims/jvm.cpp line 1272:
> 1270:
> 1271:
> 1272: // Returns the inherited_access_control_context field of the running
> thread.
There's some code in this file in
static void trace_class_resolution_impl(Klass* to_class, TRAPS) {
That does this:
while (!vfst.at_end()) {
Method* m = vfst.method();
if
(!vfst.method()->method_holder()->is_subclass_of(vmClasses::ClassLoader_klass())&&
!vfst.method()->method_holder()->is_subclass_of(access_controller_klass) &&
!vfst.method()->method_holder()->is_subclass_of(privileged_action_klass)) {
break;
}
last_caller = m;
vfst.next();
}
Is this dead code that should be removed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812677985
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 12:44:53 GMT, Coleen Phillimore wrote:
>> Sean Mullan has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 97 commits:
>>
>> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>> - Change apiNote to deprecated annotation on checkAccess methods. Change
>> method dedescription to "Does nothing".
>> - Sanitize the class descriptions of DelegationPermission and
>> ServicePermission
>>by removing text that refers to granting permissions, but avoid changes
>> that
>>affect the API specification, such as the description and format of input
>>parameters.
>> - Restored methods in RMIConnection to throw SecurityExceptions again but
>>with adjusted text that avoids the word "permission".
>> - Add text to class description of MBeanServer stating that implementations
>>may throw SecurityException if authorization doesn't allow access to
>> resource.
>> - Restore text about needing permissions from the desktop environment in the
>>getPixelColor and createScreenCapture methods.
>> - Add api note to getClassContext to use StackWalker instead and
>>add DROP_METHOD_INFO option to StackWalker.
>> - Change checkAccess() methods to be no-ops, rather than throwing
>>SecurityException.
>> - Merge
>> - Merge
>> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
>
> src/hotspot/share/prims/jvm.cpp line 1272:
>
>> 1270:
>> 1271:
>> 1272: // Returns the inherited_access_control_context field of the running
>> thread.
>
> There's some code in this file in
> static void trace_class_resolution_impl(Klass* to_class, TRAPS) {
>
> That does this:
>
>
> while (!vfst.at_end()) {
> Method* m = vfst.method();
> if
> (!vfst.method()->method_holder()->is_subclass_of(vmClasses::ClassLoader_klass())&&
>
> !vfst.method()->method_holder()->is_subclass_of(access_controller_klass) &&
>
> !vfst.method()->method_holder()->is_subclass_of(privileged_action_klass)) {
> break;
> }
> last_caller = m;
> vfst.next();
> }
>
>
> Is this dead code that should be removed?
This tracing skips ClassLoader frames, you'll continue to see these when using
Class.forName.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1812694528
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Wed, 23 Oct 2024 05:23:39 GMT, Julian Waters wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c line 53: >> >>> 51: #ifndef _WIN32 >>> 52: static MUTEX_T my_mutex = MUTEX_INIT; >>> 53: #endif >> >> The reason for no reference on windows is because of the following on >> windows: >> >> >> #define MUTEX_LOCK(x) /* FIXUP? */ >> #define MUTEX_UNLOCK(x) /* FIXUP? */ >> >> >> So looks like this mutex support is something we never got around to. I >> think your current workaround is fine. > > I'm curious now, how to implement mutex support on Windows? I think I prefer > that to just making it completely unavailable on Windows We've gone 20 years without it on Windows, so I'm inclined not to worry about the lack of support on Windows. Logging is not used often in the debug agent. I've turned in on once in a while but usually find it too noisy and hard to read. What I usually opt for is changing some of the log_message() calls to instead just use tty_message(). - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1813176284
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Wed, 23 Oct 2024 05:22:45 GMT, Julian Waters wrote:
>> src/jdk.jdi/windows/native/libdt_shmem/shmem_md.c line 47:
>>
>>> 45: {
>>> 46: void *mappedMemory;
>>> 47: // HANDLE memHandle;
>>
>> Why comment out this one but not the one at line 88? It seems they are both
>> equally problematic and are hiding the static memHandle. I'm not sure why
>> the 2nd one isn't flagged. I'd actually suggest getting rid of the static
>> memHandle. It does not seem to be needed.
>
> I wasn't sure whether the global memHandle not being used was a bug, so I
> commented out the local one. I missed the line 88 one because it wasn't
> flagged. If it really isn't needed I'll remove that one instead
I'm not sure what you mean by "that one". It's the static one that should be
removed. The local variables always hide the static, and there seems to be no
reason for the value of memHandle to survive outside of the local scope it is
used in.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1813181393
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Wed, 23 Oct 2024 21:20:45 GMT, Magnus Ihse Bursie wrote: > > I'm not sure that error was caused by this issue. That would imply that I > > had strange file attributes on files in my build directory, and I don't > > think I do. > > I can't see what else did. I think the strange attributes are injected a bit > arbitrarily by Apple. Check all your JDK files, if you dare. ;-) Oh well, I just did and you were right, I have the provenance attribute on my whole build dir atm. - PR Comment: https://git.openjdk.org/jdk/pull/21649#issuecomment-2433786667
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 12:14:24 GMT, Alan Bateman wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 97 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Change apiNote to deprecated annotation on checkAccess methods. Change >> method dedescription to "Does nothing". >> - Sanitize the class descriptions of DelegationPermission and >> ServicePermission >>by removing text that refers to granting permissions, but avoid changes >> that >>affect the API specification, such as the description and format of input >>parameters. >> - Restored methods in RMIConnection to throw SecurityExceptions again but >>with adjusted text that avoids the word "permission". >> - Add text to class description of MBeanServer stating that implementations >>may throw SecurityException if authorization doesn't allow access to >> resource. >> - Restore text about needing permissions from the desktop environment in the >>getPixelColor and createScreenCapture methods. >> - Add api note to getClassContext to use StackWalker instead and >>add DROP_METHOD_INFO option to StackWalker. >> - Change checkAccess() methods to be no-ops, rather than throwing >>SecurityException. >> - Merge >> - Merge >> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 > > test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 241: > >> 239: Properties props = System.getProperties(); >> 240: props.setProperty(JDK_LANG_PROCESS_ALLOW_AMBIGUOUS_COMMANDS, >> ""); >> 241: System.setSecurityManager(new SecurityMan()); > > I assume SecurityMan should be removed too. Yes, and the comments that say "no SM" should be updated. @RogerRiggs can you take a look at this? Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1813724467
Re: RFR: 8342858: Make target mac-jdk-bundle fails on chmod command
On Wed, 23 Oct 2024 17:26:35 GMT, Erik Joelsson wrote: > I'm not sure that error was caused by this issue. That would imply that I had > strange file attributes on files in my build directory, and I don't think I > do. I can't see what else did. I think the strange attributes are injected a bit arbitrarily by Apple. Check all your JDK files, if you dare. ;-) - PR Comment: https://git.openjdk.org/jdk/pull/21649#issuecomment-2433486088
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Tue, 22 Oct 2024 21:36:06 GMT, Mandy Chung wrote: > Reviewed test/jdk/java/lang/** and test/jdk/sun/reflect/* tests. Thanks for the comprehensive review. I have incorporated all of your comments except for removing the enum from `java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java` which is a bit more involved. I plan on posting an updated PR with these and other changes tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433572062
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 I've reviewed all jdk/java/awt and jdk/javax/imageio tests. I've either commented or proactively fixed in the jep sandbox branch. Or both. Next sync from there should resolve those. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433048411
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v21]
On Tue, 22 Oct 2024 13:53:03 GMT, Thomas Stuefe wrote: >> I will do some benchmarks > > I did SpecJBB runs with shift of 6, 8 and 10, respectively, which amounts to > Klass alignment of 64, 256 and 1K. Benchmark scores did not show a > significant pattern. I did not measure CPU stats though. > > But I still think a dynamically calculated shift makes sense, and I hesitate > to change this code at this point. I therefore would like to move this > question to followup RFEs if necessary. This code causes test errors in `CompressedClassPointersEncodingScheme.java` on s390 and PPC64. It forces the shift to `log_cacheline` which is 7 on PPC64 and 9 on s390. The test passes when we remove "s > log_cacheline && " from the condition below. In addition, it doesn't fit to the comment which claims we should avoid shifts larger than the cacheline size. This enforces shifts to be larger (or equal to) than the cacheline size. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1813304646
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 19:38:10 GMT, Harshitha Onkar wrote:
>> test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line
>> 76:
>>
>>> 74: System.out.println("java.io.tmpdir is " +
>>> System.getProperty("java.io.tmpdir"));
>>> 75:
>>> 76: if (args.length > 1) {
>>
>> The isFileCacheExpected logic does not make sense. The test sets set to use
>> the cache but then reads whether to expect it based on the args[0]. If that
>> were set to false the test will fail. So why is it there ?
>>
>> Also the messing around with exceptions at the end of the test is pointless
>
> @prrace I might have missed removing this check which was in the original
> test. The latest update to this test has two run tags but it fails when
> isFileCacheExpected is set to true.
>
> Did you mean to keep only one run tag?
> https://github.com/openjdk/jdk-sandbox/commit/1bf77a393c5756bca65760402077617d37be72d2
>
> I'll be rename the test as suggested when I update this test next.
No changes required for this test. The test was failing due to IDE config issue
of tmp dir.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1813474346
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 05:11:19 GMT, Prasanta Sadhukhan wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 97 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Change apiNote to deprecated annotation on checkAccess methods. Change >> method dedescription to "Does nothing". >> - Sanitize the class descriptions of DelegationPermission and >> ServicePermission >>by removing text that refers to granting permissions, but avoid changes >> that >>affect the API specification, such as the description and format of input >>parameters. >> - Restored methods in RMIConnection to throw SecurityExceptions again but >>with adjusted text that avoids the word "permission". >> - Add text to class description of MBeanServer stating that implementations >>may throw SecurityException if authorization doesn't allow access to >> resource. >> - Restore text about needing permissions from the desktop environment in the >>getPixelColor and createScreenCapture methods. >> - Add api note to getClassContext to use StackWalker instead and >>add DROP_METHOD_INFO option to StackWalker. >> - Change checkAccess() methods to be no-ops, rather than throwing >>SecurityException. >> - Merge >> - Merge >> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 > >> @prsadhuk Addressed review comments in the following jep486 branch commit: >> [openjdk/jdk-sandbox@d9ee496](https://github.com/openjdk/jdk-sandbox/commit/d9ee496f7349cb8beaf1e696fd430f8064baee8e) >> >> 1. test/jdk/javax/swing/JEditorPane/8080972/TestJEditor.java - Updated, >> removed redundant try-catch block >> >> 2. test/jdk/javax/swing/JPopupMenu/6691503/bug6691503.java - Repurposed >> and added back >> >> 3. test/jdk/javax/swing/JPopupMenu/6694823/bug6694823.java - Updated, >> added finally block >> >> 4. test/jdk/javax/swing/UIDefaults/6795356/TableTest.java - Added back >> >> >> Left out comments that fall into out-of-scope/clean-up for jep486. > > looks ok but awt import should have been before swing as decided, although > this again is not part of SM exercise but since you modified the tests I > thought I will say it aloud.. > @prsadhuk > > > looks ok but awt import should have been before swing as decided, although > > this again is not part of SM exercise but since you modified the tests I > > thought I will say it aloud > > Thanks! I didn't notice it happened again with the new changes. This was > probably due to IDE settings. Since this was a recent change I have updated > in the latest commit. I also didn't realise you had "changed" it. I'm finding it very painful to navigate to the related files in this huge PR. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433046255
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 12:53:12 GMT, Alan Bateman wrote:
>> src/hotspot/share/prims/jvm.cpp line 1272:
>>
>>> 1270:
>>> 1271:
>>> 1272: // Returns the inherited_access_control_context field of the running
>>> thread.
>>
>> There's some code in this file in
>> static void trace_class_resolution_impl(Klass* to_class, TRAPS) {
>>
>> That does this:
>>
>>
>> while (!vfst.at_end()) {
>> Method* m = vfst.method();
>> if
>> (!vfst.method()->method_holder()->is_subclass_of(vmClasses::ClassLoader_klass())&&
>>
>> !vfst.method()->method_holder()->is_subclass_of(access_controller_klass) &&
>>
>> !vfst.method()->method_holder()->is_subclass_of(privileged_action_klass)) {
>> break;
>> }
>> last_caller = m;
>> vfst.next();
>> }
>>
>>
>> Is this dead code that should be removed?
>
> This tracing skips ClassLoader frames, you'll continue to see these when
> using Class.forName.
but you won't see access_controller_klass or priviledged_action_klass frames,
so no need to skip them? Not sure why you'd want to skip class loader frames
here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1813400810
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Mon, 21 Oct 2024 21:12:29 GMT, Phil Race wrote:
>> Sean Mullan has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 97 commits:
>>
>> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>> - Change apiNote to deprecated annotation on checkAccess methods. Change
>> method dedescription to "Does nothing".
>> - Sanitize the class descriptions of DelegationPermission and
>> ServicePermission
>>by removing text that refers to granting permissions, but avoid changes
>> that
>>affect the API specification, such as the description and format of input
>>parameters.
>> - Restored methods in RMIConnection to throw SecurityExceptions again but
>>with adjusted text that avoids the word "permission".
>> - Add text to class description of MBeanServer stating that implementations
>>may throw SecurityException if authorization doesn't allow access to
>> resource.
>> - Restore text about needing permissions from the desktop environment in the
>>getPixelColor and createScreenCapture methods.
>> - Add api note to getClassContext to use StackWalker instead and
>>add DROP_METHOD_INFO option to StackWalker.
>> - Change checkAccess() methods to be no-ops, rather than throwing
>>SecurityException.
>> - Merge
>> - Merge
>> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
>
> test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line 76:
>
>> 74: System.out.println("java.io.tmpdir is " +
>> System.getProperty("java.io.tmpdir"));
>> 75:
>> 76: if (args.length > 1) {
>
> The isFileCacheExpected logic does not make sense. The test sets set to use
> the cache but then reads whether to expect it based on the args[0]. If that
> were set to false the test will fail. So why is it there ?
>
> Also the messing around with exceptions at the end of the test is pointless
@prrace I might have missed removing this check which was in the original test.
The latest update to this test has two run tags but it fails when
isFileCacheExpected is set to true.
Did you mean to keep only one run tag?
https://github.com/openjdk/jdk-sandbox/commit/1bf77a393c5756bca65760402077617d37be72d2
I'll be rename the test as suggested when I update this test next.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1813429265
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v50]
On Tue, 22 Oct 2024 16:22:20 GMT, Roman Kennke wrote: > @egahlin / @mgronlun could you please review the JFR parts of this PR? One > change is for getting the right prototype header, the other is for avoiding > an endless loop/assert in a corner case. JFR changes look reasonable. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2433263488
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v50]
On Tue, 22 Oct 2024 16:19:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update copyright > - Avoid assert/endless-loop in JFR code Marked as reviewed by egahlin (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2389952721
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
On Mon, 21 Oct 2024 06:11:26 GMT, Quan Anh Mai wrote: >> Jasmine Karthikeyan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address some changes from code review > > src/hotspot/share/opto/phaseX.cpp line 2277: > >> 2275: >> 2276: // Try to find an existing version of the same node >> 2277: Node* existing = _igvn->hash_find_insert(n); > > I think it would be easier if you have a switch in `gvn` that says you passed > the point of doing `Ideal`, moving forward you will probably want to have a > `IdealLowering` to transform nodes during this phase. `Identity` I think is > fine since it returns an existing node. Ah, do you mean having a method in `Node` that holds the lowering code? I was originally planning on doing it this way, but I think it would pose some problems where certain nodes' `Lower()` methods would only be overridden on certain backends, which would become messy. One of my goals was to keep the lowering code separate from shared code, so new lowerings could be implemented by just updating the main `lower_node` function in the backend. About GVN, I think it makes sense to do it in a separate phase because GVN is used quite generally whereas lowering is only done once. Since the `transform_old` function in IGVN is pretty complex as well, I think it's simpler to just implement `Value()` and GVN separately. Thinking on it more I think Identity is probably a good idea too, since as you mention it can't introduce new nodes into the graph. Mainly I wanted to avoid the case where `Ideal()` could fold a lowered graph back into the original form, causing an infinite loop. - PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1814136201
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
On Wed, 23 Oct 2024 17:39:22 GMT, Quan Anh Mai wrote: >> Because lowering is a transformation that increases the complexity of the >> graph. >> >> - A `d = ExtractD(z, 4)` expanded into `x = VectorExtract(z, 2); d = >> ExtractD(x, 0)` increases the number of nodes by 1. >> - A logic cone transformed into a `MacroLogicV` introduces another kind of >> node that may not be recognized by other nodes. >> >> As a result, we should do this as the last step when other transformation >> has finished their jobs. For the concerns regarding loop body size, we still >> have a function in `Matcher` for that purpose. > > Another reason is that lowering being done late allows us to have more > freedom to break some invariants of the nodes, such as looking through > `VectorReinterpret`. An example is this (really crafted) case: > > Int256Vector v; > int a = v.lane(5); > float b = v.reinterpretAsFloats().lane(7); > > This would be transformed into: > > vector v; > vector v0 = VectorExtract(v, 1); > int a = ExtractI(v0, 1); > vector v1 = VectorReinterpret(v, ); > vector v2 = VectorExtract(v1, 1); > float b = ExtractF(v2, 3); > > By allowing lowering to look through `VectorReinterpret` and break the > invariant of `Extract` nodes that the element types of their inputs and > outputs must be the same, we can `gvn` `v1` and `v`, `v2` and `v0`. Simplify > the graph: > > vector v; > vector v0 = VectorExtract(v, 1); > int a = ExtractI(v0, 1); > float b = ExtractF(v0, 3); > Because lowering is a transformation that increases the complexity of the > graph. > > * A `d = ExtractD(z, 4)` expanded into `x = VectorExtract(z, 2); d = > ExtractD(x, 0)` increases the number of nodes by 1. > * A logic cone transformed into a `MacroLogicV` introduces another kind of > node that may not be recognized by other nodes. > > As a result, we should do this as the last step when other transformation has > finished their jobs. For the concerns regarding loop body size, we still have > a function in `Matcher` for that purpose. Yes, you rightly pointed out, given the fact that lowering in some cases may significantly impact the graph shape it should be accounted by loop optimizations. Unrolling decisions are based on loop body size and a rudimentary cost model e.g. macro logic optimization which folds entire logic tree into one x86 specific lowered IR should promote unrolling. - PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1814134951
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
On Wed, 23 Oct 2024 07:57:05 GMT, Jatin Bhateja wrote:
>> Jasmine Karthikeyan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address some changes from code review
>
> src/hotspot/share/opto/phaseX.cpp line 2301:
>
>> 2299: while(_igvn->_worklist.size() != 0) {
>> 2300: Node* n = _igvn->_worklist.pop();
>> 2301: Node* new_node = lower_node(n);
>
> _PhaseLowring::lower_node_ may do complex transformation where by replacing a
> graph pallet rooted at current node by another pallet. For each newly created
> node in new pallet, it should make sure to either directly run
> _igvn.transform, thereby triggering Ideal / Identity / Value sub-passed over
> it, OR insert the node into _igvn.worklist for lazy processing, in latter
> case you are consuming entire worklist after running over only Value
> transforms before existing the lowering phase.
I think we shouldn't run `Ideal` on the graph, because there is a chance that
it could undo the lowering changes that we just did. This gives lowering more
freedom to change the graph in different ways that would otherwise be undone by
Ideal. We run `Value` on all of the transformed nodes mainly so the types table
is accurate, so we can call upon the type of any node during lowering.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1814139289
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Fri, 30 Aug 2024 07:37:35 GMT, Thomas Stuefe wrote: >> make/Images.gmk line 135: >> >>> 133: # >>> 134: # Param1 - VM variant (e.g., server, client, zero, ...) >>> 135: # Param2 - _nocoops, _coh, _nocoops_coh, or empty >> >> The -XX:+UseCompactObjectHeaders ssems to incompatible withe zero vm. The >> zero vm build start failing while generating shared archive with >> +UseCompactObjectHeaders. Generation should be disabled by default for zero >> to don't break the build. > > No, zero works with +COH, but a small change is needed. I'll post a > suggestion inline. no objection anymore - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1814259543
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
> Hi all, > This patch adds a new pass to consolidate lowering of complex > backend-specific code patterns, such as `MacroLogicV` and the optimization > proposed by #21244. Moving these optimizations to backend code can simplify > shared code, while also making it easier to develop more in-depth > optimizations. The linked bug has an example of a new optimization this could > enable. The new phase does GVN to de-duplicate nodes and calls nodes' > `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid > undoing any changes done during lowering. It also reuses the IGVN worklist to > avoid needing to re-create the notification mechanism. > > In this PR only the skeleton code for the pass is added, moving `MacroLogicV` > to this system will be done separately in a future patch. Tier 1 tests pass > on my linux x64 machine. Feedback on this patch would be greatly appreciated! Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision: Address some changes from code review - Changes: - all: https://git.openjdk.org/jdk/pull/21599/files - new: https://git.openjdk.org/jdk/pull/21599/files/0ce85525..7fbc4509 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21599&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21599&range=00-01 Stats: 27 lines in 9 files changed: 12 ins; 2 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/21599.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21599/head:pull/21599 PR: https://git.openjdk.org/jdk/pull/21599
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
On Thu, 24 Oct 2024 02:13:20 GMT, Jasmine Karthikeyan wrote: > I think we shouldn't run `Ideal` on the graph, because there is a chance that > it could undo the lowering changes that we just did. This gives lowering more > freedom to change the graph in different ways that would otherwise be undone > by Ideal. We run `Value` on all of the transformed nodes mainly so the types > table is accurate, so we can call upon the type of any node during lowering. We need to preserve lowering canonicalizations, but lowered graph is susceptible to further idealizations, else we could do lowering during final graph reshaping just before matching. - PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1814176800
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]
On Mon, 21 Oct 2024 12:55:39 GMT, Magnus Ihse Bursie wrote: >> Jasmine Karthikeyan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address some changes from code review > > Build changes look good (but would be slightly better without the extra blank > line). I have not reviewed the actual hotspot changes. Thanks for looking at the build changes, @magicus! I've pushed a commit that removes the extra newline in the makefiles and adds newlines to the ends of files that were missing them. Thanks for taking a look as well, @merykitty and @jatin-bhateja! I've pushed a commit that should address the code suggestions and left some comments as well. - PR Comment: https://git.openjdk.org/jdk/pull/21599#issuecomment-2434090294
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did Thanks Weijun for the ok on security (Though it still makes me uneasy because it smells like a bug, but I guess it's alright). I haven't pushed yet to remove the changes to the other files in this Pull Request because the way I did it I'd have to force push and that would destroy all the review comments, I'll do that once the comments have been fully addressed - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2434176106
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Wed, 23 Oct 2024 16:51:23 GMT, Chris Plummer wrote: >> I wasn't sure whether the global memHandle not being used was a bug, so I >> commented out the local one. I missed the line 88 one because it wasn't >> flagged. If it really isn't needed I'll remove that one instead > > I'm not sure what you mean by "that one". It's the static one that should be > removed. The local variables always hide the static, and there seems to be no > reason for the value of memHandle to survive outside of the local scope it is > used in. Oh, I was referring to the static global one. In that case I'll remove it (Or rather, comment it so it isn't lost to time and someone can bring it back if need be) - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1814187989
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
On Thu, 24 Oct 2024 03:31:31 GMT, Julian Waters wrote: >> I'm not sure what you mean by "that one". It's the static one that should be >> removed. The local variables always hide the static, and there seems to be >> no reason for the value of memHandle to survive outside of the local scope >> it is used in. > > Oh, I was referring to the static global one. In that case I'll remove it (Or > rather, comment it so it isn't lost to time and someone can bring it back if > need be) I would just remove the static. I don't see any reason for it to exist. - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1814375764
