Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8207011: Remove uses of the register storage class specifier #11

Closed
wants to merge 1 commit into from

Conversation

dongbohe
Copy link
Member

@dongbohe dongbohe commented Mar 17, 2022

Hi,

Please review the backport of JDK-8207011 to 8u.
Bug: https://bugs.openjdk.java.net/browse/JDK-8207011
Original commit: https://git.openjdk.java.net/jdk11u-dev/commit/98fb4f5e18a58727f51e00e3c08c0f5eac6748ec
8u webrev: http://cr.openjdk.java.net/~dongbohe/8207011/webrev.01/

This patch fixes build failure with gcc 11. Patch doesn’t apply cleanly, because JDK-8188813[1], JDK-8204301[2], JDK-8041415 [3], and JDK-8186089[4] does not exist in 8.

  • JDK-8188813 added register in orderAccess_aix_ppc.inline.hpp, orderAccess_linux_ppc.inline.hpp, orderAccess_linux_s390.inline.hpp,
    and JDK-8204301 renamed these files to orderAccess_aix_ppc.inline.hpp, orderAccess_aix_ppc.hpp, orderAccess_linux_s390.hpp.
  • JDK-8041415 changed uint32 to uint32_t.
  • JDK-8186089 moved arena from allocation.cpp to arena.cpp.

Before patch:
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/home/jiangshunningy/install/libexec/gcc/aarch64-unknown-linux-gnu/11.2.0/lto-wrapper
Target: aarch64-unknown-linux-gnu
Configured with: ../configure --prefix=/home/jiangshunningy/install --enable-languages=c,c++,fortran,go
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 11.2.0 (GCC)

$ bash configure
$ make images
/home/jiangshunningy/hedongbo/jdk8u-dev/hotspot/src/share/vm/adlc/dict2.cpp:286:17: error: ISO C++17 does not allow ‘register’ storage class specifier [-Werror=register]
286 | register char c, k = 0;
| ^
/home/jiangshunningy/hedongbo/jdk8u-dev/hotspot/src/share/vm/adlc/dict2.cpp:286:20: error: ISO C++17 does not allow ‘register’ storage class specifier [-Werror=register]
286 | register char c, k = 0;
| ^
/home/jiangshunningy/hedongbo/jdk8u-dev/hotspot/src/share/vm/adlc/dict2.cpp:287:16: error: ISO C++17 does not allow ‘register’ storage class specifier [-Werror=register]
287 | register int sum = 0;
| ^~~
/home/jiangshunningy/hedongbo/jdk8u-dev/hotspot/src/share/vm/adlc/dict2.cpp:288:24: error: ISO C++17 does not allow ‘register’ storage class specifier [-Werror=register]
288 | register const char *s = (const char *)t;
| ^

After patch:
Build succeeded.

[1] https://bugs.openjdk.java.net/browse/JDK-8188813
[2] https://bugs.openjdk.java.net/browse/JDK-8204301
[3] https://bugs.openjdk.java.net/browse/JDK-8041415
[4] https://bugs.openjdk.java.net/browse/JDK-8186089

This is PR for backport before move to github: https://mail.openjdk.java.net/pipermail/jdk8u-dev/2022-February/014579.html

Thanks,
hedongbo


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8207011: Remove uses of the register storage class specifier

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk8u-dev pull/11/head:pull/11
$ git checkout pull/11

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk8u-dev/pull/11.diff

Backport-of: 6ffd168ad145af0b6fdd72f3130e79b6a1bb9ad0
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 17, 2022

👋 Welcome back dongbohe! 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 openjdk bot changed the title Backport 6ffd168ad145af0b6fdd72f3130e79b6a1bb9ad0 8207011: Remove uses of the register storage class specifier Mar 17, 2022
@openjdk
Copy link

openjdk bot commented Mar 17, 2022

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

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Mar 17, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2022

Webrevs

@dongbohe
Copy link
Member Author

It seems that some of the information described on the PR is not displayed in the email(https://mail.openjdk.java.net/pipermail/jdk8u-dev/2022-March/014654.html).

@gnu-andrew
Copy link
Member

This is an enhancement/cleanup fix and not appropriate for backporting to 8u.

Besides, this isn't the actual issue here. The baseline for 8u is C++98 (well, gnu++98 for GCC) where register is not deprecated. Newer versions of GCC have a later C++ standard as default so it has to be explicitly set (see JDK-8151841)

The problem you're seeing here is that the ADLC code is not being passed that option. Indeed, it is not passed any of the extra C and C++ flags that may be specified to the build. This has gone unnoticed thus far because ADLC is a build tool that is used to generate files during the build and is not part of the shipped product.

I already have a bug for this - JDK-8281098 - and a tested fix. I've not yet submitted it upstream as I'd like at least the build testing on PRs to be in place - PR #3 - so we can reduce the (already unlikely) chance it breaks anything.

I would appreciate it if you would withdraw this PR and wait for JDK-8281098 to be integrated. That should happen soon.

@dongbohe
Copy link
Member Author

his is an enhancement/cleanup fix and not appropriate for backporting to 8u.

Besides, this isn't the actual issue here. The baseline for 8u is C++98 (well, gnu++98 for GCC) where register is not deprecated. Newer versions of GCC have a later C++ standard as default so it has to be explicitly set (see JDK-8151841)

The problem you're seeing here is that the ADLC code is not being passed that option. Indeed, it is not passed any of the extra C and C++ flags that may be specified to the build. This has gone unnoticed thus far because ADLC is a build tool that is used to generate files during the build and is not part of the shipped product.

Thanks for reminding me of this and explaining the issue in detail.

I already have a bug for this - JDK-8281098 - and a tested fix. I've not yet submitted it upstream as I'd like at least the build testing on PRs to be in place - PR #3 - so we can reduce the (already unlikely) chance it breaks anything.

I would appreciate it if you would withdraw this PR and wait for JDK-8281098 to be integrated. That should happen soon.

Glad to hear that, I'll close this PR now and look forward to JDK-8281098 being integrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
2 participants