Skip to content

Conversation

@magicus
Copy link
Member

@magicus magicus commented Oct 28, 2024

This is the implementation of JEP 479: Remove the Windows 32-bit x86 Port.

This is the summary of JEP 479:

Remove the source code and build support for the Windows 32-bit x86 port. This port was deprecated for removal in JDK 21 with the express intent to remove it in a future release.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a JEP request to be targeted
  • Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
  • Change requires CSR request JDK-8339784 to be approved

Issues

  • JDK-8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port (Enhancement - P3)
  • JDK-8330623: JEP 479: Remove the Windows 32-bit x86 Port (JEP)
  • JDK-8339784: Implement JEP 479: Remove the Windows 32-bit x86 Port (CSR)

Reviewers

Reviewers without OpenJDK IDs

  • @swesonga (no known openjdk.org user name / role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21744/head:pull/21744
$ git checkout pull/21744

Update a local copy of the PR:
$ git checkout pull/21744
$ git pull https://git.openjdk.org/jdk.git pull/21744/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21744

View PR using the GUI difftool:
$ git pr show -t 21744

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21744.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2024

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Oct 28, 2024

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port

Reviewed-by: kbarrett, kvn, stuefe, shade, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 57 new commits pushed to the master branch:

  • a4e2c20: 8343344: Windows attach logic failed to handle a failed open on a pipe
  • 63eb485: 8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304
  • db85090: 8338411: Implement JEP 486: Permanently Disable the Security Manager
  • c12b386: 8338007: [JVMCI] ResolvedJavaMethod.reprofile can crash ciMethodData
  • 81752c4: 8338565: Test crashed: assert(is_path_empty()) failed: invariant
  • e5eaa7f: 8343946: JFR: Wildcard should only work with COUNT for 'jfr view'
  • 2989d87: 8343805: RISC-V: JVM crashes on startup when disabling compressed instructions
  • 78b8015: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
  • 8a2a75e: 8339892: Several security shell tests don't set TESTJAVAOPTS
  • 50b6e41: 8300732: Whitebox functions for Metaspace test should use byte size
  • ... and 47 more: https://git.openjdk.org/jdk/compare/0c281acfb4c87436096cb562d70f800dffa3671a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk
Copy link

openjdk bot commented Oct 28, 2024

@magicus The following labels will be automatically applied to this pull request:

  • build
  • client
  • core-libs
  • graal
  • hotspot
  • nio
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org nio nio-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 28, 2024
@magicus
Copy link
Member Author

magicus commented Oct 28, 2024

For this patch, I have removed the parts of the build that I knew of that were related to 32-bit Windows. Furthermore, I have searched for all defines across the code base that are related to 32-bit Windows, and removed the parts of the code that is no longer relevant. I have also made a cross-codebase search for terms like "win" and "32" and glanced through the results (that was a huge list) to see if I could spot anything that might need attention.

There might of course still be special code that was developed to take care of Windows 32-bit that is no longer needed, but that is hard to find automatically. If anyone knows about some particular code, please let me know!

Most of the code was trivial to handle, but there are a few instances where I'd like some input from code owners. I've marked these with FIXME in the patch.

@magicus
Copy link
Member Author

magicus commented Oct 28, 2024

/jep 479

@openjdk
Copy link

openjdk bot commented Oct 28, 2024

@magicus
This pull request will not be integrated until the JEP-479 has been targeted.

@openjdk openjdk bot added the jep label Oct 28, 2024
@magicus magicus marked this pull request as ready for review October 28, 2024 18:20
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 28, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 28, 2024

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursory review, sometimes with my 32-bit x86 maintainer hat on :)

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the build system parts.

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 28, 2024

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hotspot changes look good. May be some further cleanup possible. A couple of queries.

Thanks

@openjdk
Copy link

openjdk bot commented Oct 29, 2024

@magicus this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout impl-JEP-479
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@alexmenkov
Copy link

Can someone confirm that use of __stdcall has no affect on name decorations, as there is no mention here about anything being ignored:

https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170

I would have expected that if argument passing needs to use the stack then the decorated name would still need to encode that somehow.

In the page you mentioned:

Format of a C decorated name
The form of decoration for a C function depends on the calling convention used in its declaration, as shown in the following table. It's also the decoration format that's used when C++ code is declared to have extern "C" linkage. The default calling convention is __cdecl. In a 64-bit environment, C or extern "C" functions are only decorated when using the __vectorcall calling convention.

@TheShermanTanker
Copy link
Contributor

TheShermanTanker commented Nov 8, 2024

Can someone confirm that use of __stdcall has no affect on name decorations, as there is no mention here about anything being ignored:

https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170

I would have expected that if argument passing needs to use the stack then the decorated name would still need to encode that somehow.

Not __stdcall: https://godbolt.org/z/nvjTP5WPc
__stdcall: https://godbolt.org/z/1KejW44vY

@dholmes-ora
Copy link
Member

In the page you mentioned:

@alexmenkov that is for C functions, not C++.

@dholmes-ora
Copy link
Member

Thanks @TheShermanTanker . I see the arguments do affect the encoding but the __stdcall makes no difference.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing my "changes requested" status

EDIT: didn't work. Seems I can't clear it except by approving. So just ignore it.

@magicus
Copy link
Member Author

magicus commented Nov 8, 2024

@shipilev Could you consider accepting the existing __stdcall changes in this PR? That seems like the easiest way out of satisfying everyones opinions here..

As I said, I think they are just as safe as any other changes -- the only difference is that it is perhaps not as well-known in the community that they only affect x86.

@alexmenkov
Copy link

In the page you mentioned:

@alexmenkov that is for C functions, not C++.

And also for extern "C" (AFAIU we export all C++ functions as extern "C")

@shipilev
Copy link
Member

shipilev commented Nov 8, 2024

@shipilev Could you consider accepting the existing __stdcall changes in this PR? That seems like the easiest way out of satisfying everyones opinions here..

Sure, fine. I am reading the whole thing again.

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 8, 2024
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 8, 2024
Copy link

@kimbarrett kimbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve.

@magicus magicus requested a review from erikj79 November 8, 2024 21:41
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good to me.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed jep labels Nov 12, 2024
@magicus
Copy link
Member Author

magicus commented Nov 13, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Nov 13, 2024

Going to push as commit 79345bb.
Since your change was applied there have been 62 commits pushed to the master branch:

  • 2eeaa57: 8343944: C2: MinLNode::add_ring() computes _widen wrongly leading to an endless widening/compilation
  • e9ede46: 8343508: Parallel: Use ordinary klass accessor in verify_filler_in_dense_prefix
  • c78de7b: 8343964: RISC-V: Improve PrintOptoAssembly output for loadNKlassCompactHeaders node
  • eb40a88: 8343430: RISC-V: C2: Remove old trampoline call
  • b26e495: 8343801: Change string printed by nsk_null_string for null strings
  • a4e2c20: 8343344: Windows attach logic failed to handle a failed open on a pipe
  • 63eb485: 8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304
  • db85090: 8338411: Implement JEP 486: Permanently Disable the Security Manager
  • c12b386: 8338007: [JVMCI] ResolvedJavaMethod.reprofile can crash ciMethodData
  • 81752c4: 8338565: Test crashed: assert(is_path_empty()) failed: invariant
  • ... and 52 more: https://git.openjdk.org/jdk/compare/0c281acfb4c87436096cb562d70f800dffa3671a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 13, 2024
@openjdk openjdk bot closed this Nov 13, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 13, 2024
@openjdk
Copy link

openjdk bot commented Nov 13, 2024

@magicus Pushed as commit 79345bb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@magicus magicus deleted the impl-JEP-479 branch November 13, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build build-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated nio nio-dev@openjdk.org serviceability serviceability-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.