8315020: The macro definition for LoongArch64 zero build is not accurate. #489

xiangzhai · 2024-04-28T06:48:33Z

Hi,

I'd like to backport this patch to jdk8u. common/autoconf/platform.m4 and hotspot/src/os/linux/vm/os_linux.cpp do not apply cleanly due to context difference, but it is easy to resolve them manually.

A native build on LoongArch hardware is tested.

Debian: https://mail.openjdk.org/pipermail/jdk8u-dev/2024-April/018378.html

$ ./build/images/j2sdk-image/bin/java -version
openjdk version "1.8.0_412"
OpenJDK Runtime Environment (build 1.8.0_412-8u412-ga-1-b08)
OpenJDK 64-Bit Zero VM (build 25.412-b08, interpreted mode)

Loongnix Desktop:

$ ./build/linux-loongarch64-normal-zero-release/images/j2sdk-image/bin/java -version
openjdk version "1.8.0_422-internal"
OpenJDK Runtime Environment (build 1.8.0_422-internal-zhaixiang_2024_04_28_10_57-b00)
OpenJDK 64-Bit Zero VM (build 25.422-b00, interpreted mode)

The risk of the downport is low.

Thanks,
Leslie Zhai

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
JDK-8315020 needs maintainer approval
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8315020: The macro definition for LoongArch64 zero build is not accurate. (Bug - P4 - Requested)

Reviewers

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

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/489/head:pull/489
$ git checkout pull/489

Update a local copy of the PR:
$ git checkout pull/489
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/489/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 489

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/489.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-28T06:48:56Z

👋 Welcome back lzhai! 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 · 2024-04-28T06:50:05Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2024-04-28T06:53:30Z

Webrevs

01: Full - Incremental (6728fd32)
00: Full (681fcbb9)

xiangzhai · 2024-04-28T07:03:33Z

Hi @theaoqi @gnu-andrew

Could you review and sponsor the patch please?

Thanks,
Leslie Zhai

xiangzhai · 2024-04-28T07:03:45Z

/approval request allow Zero build on LoongArch

openjdk · 2024-04-28T07:04:43Z

@xiangzhai
8315020: The approval request has been created successfully.

mirabilos · 2024-04-28T16:07:36Z

common/autoconf/platform.m4


  # ZERO_ARCHDEF is used to enable architecture-specific code
  case "${OPENJDK_TARGET_CPU}" in
+    loongarch64)  ZERO_ARCHDEF=LOONGARCH64 ;;


This is not necessary, line 390 already does that.

But the line 390 is for x86, isn't it? Please point out my fault!

Thanks,
Leslie Zhai

The other 390 ;-) 391 after the patch.

The *) case.

mirabilos · 2024-04-28T16:07:57Z

hotspot/src/os/linux/vm/os_linux.cpp

  #elif  (defined AARCH64)
    static  Elf32_Half running_arch_code=EM_AARCH64;
-  #elif  (defined LOONGARCH)
+  #elif  (defined LOONGARCH64)


+1, this fixes the compile issue I reported.

mlbridge · 2024-04-29T04:15:03Z

Mailing list message from Thorsten Glaser on jdk8u-dev:

On Sun, 28 Apr 2024, Leslie Zhai wrote:

There?s an unnecessary line in there, I noted that on the web thingy.

The failing s390x build is because this is still using
Debian 10 (buster), which has recently been archived.

Do an s!http://(httpredir|deb).debian.org!http://archive.debian.org!g
to fix that.

bye,
//mirabilos
--
Infrastrukturexperte ? Qvest Digital AG
Am Dickobskreuz 10, D-53121 Bonn ? https://www.qvest-digital.com/
Telephon +49 228 54881-393 ? Fax: +49 228 54881-235
HRB AG Bonn 18196 ? USt-ID (VAT): DE274355441
Vorstand: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg
Vorsitzender Aufsichtsrat: Peter N?then

bridgekeeper · 2024-06-04T21:34:34Z

@xiangzhai This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

gnu-andrew

The PR title should be "Backport 725ec0ce1b463b21cd4c5287cf4ccbee53ec7349" so this is correctly recognised as a backport.

I agree with @mirabilos that the platform.m4 change is unneeded. That block is only needed where ZERO_ARCHDEF and OPENJDK_TARGET_CPU_LEGACY_LIB differ e.g. ppc vs. ppc32 or x86_64 vs. amd64. The ppc64 one is also unnecessary but I wouldn't change this at this juncture.

I'm also leaning towards thinking we should file this against https://github.com/openjdk/jdk8u to get it in as a fix for July rather than waiting for October. The risk to other architectures should be zero, while this makes the new platform build. Sorry for not getting to this earlier so it would go in before rampdown. Thoughts?

Did the original version build on some LoongArch64 platforms? I'm wondering how this got through the original testing.

openjdk · 2024-06-13T01:27:50Z

This backport pull request has now been updated with issue from the original commit.

xiangzhai · 2024-06-13T12:34:55Z

Hi @gnu-andrew

Could you review my patch again please?

Thanks,
Leslie Zhai

gnu-andrew · 2024-06-15T16:35:52Z

Hi @gnu-andrew

Could you review my patch again please?

Thanks, Leslie Zhai

Updated patch looks fine, but can we please open this as a PR against https://github.com/openjdk/jdk8u ? We can then get this fixed in the July release, but we only have next week to do this. Thanks.

xiangzhai · 2024-06-17T01:04:17Z

Hi @gnu-andrew
Could you review my patch again please?
Thanks, Leslie Zhai

Updated patch looks fine, but can we please open this as a PR against https://github.com/openjdk/jdk8u ? We can then get this fixed in the July release, but we only have next week to do this. Thanks.

Could you review the patch please? openjdk/jdk8u#56

Thanks,
Leslie Zhai

jerboaa · 2024-06-17T09:55:32Z

This one needs to close if it gets integrated with openjdk/jdk8u#56

Backport 725ec0ce1b463b21cd4c5287cf4ccbee53ec7349

681fcbb

openjdk bot changed the title ~~8315020: The macro definition for LoongArch64 zero build is not accurate~~ 8315020: The macro definition for LoongArch64 zero build is not accurate. Apr 28, 2024

openjdk bot added the rfr Pull request is ready for review label Apr 28, 2024

openjdk bot added the approval Requires approval; will be removed when approval is received label Apr 28, 2024

mirabilos suggested changes Apr 28, 2024

View reviewed changes

gnu-andrew reviewed Jun 12, 2024

View reviewed changes

Remove unnecessary ZERO_ARCHDEF

6728fd3

xiangzhai requested review from gnu-andrew and mirabilos June 13, 2024 01:24

xiangzhai changed the title ~~8315020: The macro definition for LoongArch64 zero build is not accurate.~~ Backport 725ec0ce1b463b21cd4c5287cf4ccbee53ec7349 Jun 13, 2024

openjdk bot changed the title ~~Backport 725ec0ce1b463b21cd4c5287cf4ccbee53ec7349~~ 8315020: The macro definition for LoongArch64 zero build is not accurate. Jun 13, 2024

openjdk bot added the backport Port of a pull request already in a different code base label Jun 13, 2024

mirabilos approved these changes Jun 13, 2024

View reviewed changes

xiangzhai mentioned this pull request Jun 17, 2024

8315020: The macro definition for LoongArch64 zero build is not accurate. openjdk/jdk8u#56

Closed

4 tasks

xiangzhai closed this Jun 17, 2024

8315020: The macro definition for LoongArch64 zero build is not accurate. #489

8315020: The macro definition for LoongArch64 zero build is not accurate. #489

Uh oh!

Conversation

xiangzhai commented Apr 28, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 28, 2024

Uh oh!

openjdk bot commented Apr 28, 2024

Uh oh!

mlbridge bot commented Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

xiangzhai commented Apr 28, 2024

Uh oh!

xiangzhai commented Apr 28, 2024

Uh oh!

openjdk bot commented Apr 28, 2024

Uh oh!

mirabilos Apr 28, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangzhai Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirabilos Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

mirabilos Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

mirabilos Apr 28, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Apr 29, 2024

Uh oh!

bridgekeeper bot commented Jun 4, 2024

Uh oh!

gnu-andrew left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 13, 2024

Uh oh!

xiangzhai commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnu-andrew commented Jun 15, 2024

Uh oh!

xiangzhai commented Jun 17, 2024

Uh oh!

jerboaa commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

xiangzhai commented Apr 28, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 28, 2024 •

edited

Loading

mirabilos Apr 28, 2024 •

edited by bridgekeeper bot

Loading

xiangzhai Jun 13, 2024 •

edited

Loading

mirabilos Apr 28, 2024 •

edited by bridgekeeper bot

Loading

xiangzhai commented Jun 13, 2024 •

edited

Loading