8339331: GCC fortify error in vm_version_linux_aarch64.cpp #22655

sendaoYan · 2024-12-10T03:44:03Z

Hi all,
The file src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp reported nullptr compile warning by gcc14 with fastdebug configure. I think it's false positive, since the statement 'assert(buf != nullptr, "invalid argument");' has make sure the buf should not be nullptr before execute '::read(int, void*, int)'. The call chain is void VM_Version::initialize_cpu_information(void) -> void VM_Version::get_compatible_board(char *buf, int buflen) -> static bool read_fully(const char *fname, char *buf, size_t buflen), *buf is defined as char Abstract_VM_Version::_cpu_desc[4096] = {0}; and then right shift 8 bytes, so *buf should not be nullptr.
This PR use PRAGMA_NONNULL_IGNORED suppress false positive gcc compile warning to make fastdebug configure make success on linux-aarch64 by gcc14.2.0.
Risk is low.

Additional testing:

jtreg tests(include tier1/2/3 etc.) on linux-x64 with release build
jtreg tests(include tier1/2/3 etc.) on linux-x64 with fastdebug build
jtreg tests(include tier1/2/3 etc.) on linux-aarch64 with release build
jtreg tests(include tier1/2/3 etc.) on linux-aarch64 with fastdebug build

Progress

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

Issue

JDK-8339331: GCC fortify error in vm_version_linux_aarch64.cpp (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22655

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

Using diff file

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

Using Webrev

Link to Webrev Comment

… warning by gcc14

bridgekeeper · 2024-12-10T03:44:58Z

👋 Welcome back syan! 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-12-10T03:45:18Z

@sendaoYan 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:

8339331: GCC fortify error in vm_version_linux_aarch64.cpp

Reviewed-by: dholmes

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 140 new commits pushed to the master branch:

ea50c54: 8321818: vmTestbase/nsk/stress/strace/strace015.java failed with 'Cannot read the array length because "" is null'
c0f0b8e: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest
f3e2f88: 8346394: Bundled freetype library needs to have JNI_OnLoad for static builds
414eb6b: 8338714: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with JTREG_TEST_THREAD_FACTORY=Virtual
dc71e8c: 8342995: Enhance Attach API to support arbitrary length arguments - Linux
4f44cf6: 8341481: [perf] vframeStreamCommon constructor may be optimized
390b205: 8346048: test/lib/containers/docker/DockerRunOptions.java uses addJavaOpts() from ctor
03821d9: 8346195: Fix static initialization problem in GDIHashtable
a5503fb: 8346432: java.lang.foreign.Linker comment typo
fbd76ca: 8337016: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java gets Metaspace OOM
... and 130 more: https://git.openjdk.org/jdk/compare/69e664de14d1f9d66447937d494da8bf971ac5fe...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 · 2024-12-10T03:45:52Z

@sendaoYan The following label will be automatically applied to this pull request:

hotspot-runtime

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

mlbridge · 2024-12-10T03:49:19Z

Webrevs

sendaoYan · 2024-12-10T04:59:47Z

/issue JDK-8339331

openjdk · 2024-12-10T05:00:25Z

@sendaoYan
Adding additional issue to issue list: 8339331: GCC fortify error in vm_version_linux_aarch64.cpp.

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

sendaoYan · 2024-12-10T12:40:26Z

GHA report test gc/shenandoah/mxbeans/TestPauseNotifications.java#generational fails on windows-x64, which I create a new jbs issue JDK-8345895 to record it. This PR touch file vm_version_linux_aarch64.cpp will not effect x64 in theory, I think the test failure on windows-x64 unralated to this PR.

dholmes-ora

It annoys me that we have to add runtime overhead to release builds to suppress this kind of warning. This code only has internal callers, all of which should be known to not pass a null buf, so the assert should suffice here.

But given the very limited use of the affected function I will approve it.

Thanks

kimbarrett

We've never been able to reproduce this at Oracle. We're using vanilla builds
of gcc. It might be that Fedora is applying a patch that leads to this. In
JDK-8316234 it was suggested there might be a gcc bug involved:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87489
Having read through that bug, I'm inclined to agree that's probably involved.
Much as I hate warning suppresions, I think a better solution here is to use
PRAGMA_NONNULL_IGNORED, along with a comment referring to that gcc bug, and
also mentioning that it might require some distro-specific gcc patch, since
not seen with vanilla gcc release.

…omments about the gcc bug

sendaoYan · 2024-12-17T10:01:59Z

I think a better solution here is to use PRAGMA_NONNULL_IGNORED

Thanks, PR has been change to use PRAGMA_NONNULL_IGNORED to suppress warning, and the comments has been added.

theRealAph · 2024-12-17T17:14:31Z

We've never been able to reproduce this at Oracle. We're using vanilla builds of gcc. It might be that Fedora is applying a patch that leads to this. In JDK-8316234 it was suggested there might be a gcc bug involved: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87489 Having read through that bug, I'm inclined to agree that's probably involved. Much as I hate warning suppresions, I think a better solution here is to use PRAGMA_NONNULL_IGNORED, along with a comment referring to that gcc bug, and also mentioning that it might require some distro-specific gcc patch, since not seen with vanilla gcc release.

Mmm, I see. Agreed.

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

dholmes-ora

LGTM.

Thanks

sendaoYan · 2024-12-18T06:33:24Z

GHA report 1 failure:

macos-aarch64 / test - Test (tier1) job fails at Checout the JDK source stage "protocol error: bad pack header", it seems environmental issue or infrastructure issue, unrelated to this PR.

sendaoYan · 2024-12-18T06:34:31Z

Thanks all for the reviews and advices.

/integrate

openjdk · 2024-12-18T06:35:31Z

Going to push as commit 842f801.
Since your change was applied there have been 141 commits pushed to the master branch:

4533109: 8345911: Enhance error message when IncompatibleClassChangeError is thrown for sealed class loading failures
ea50c54: 8321818: vmTestbase/nsk/stress/strace/strace015.java failed with 'Cannot read the array length because "" is null'
c0f0b8e: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest
f3e2f88: 8346394: Bundled freetype library needs to have JNI_OnLoad for static builds
414eb6b: 8338714: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with JTREG_TEST_THREAD_FACTORY=Virtual
dc71e8c: 8342995: Enhance Attach API to support arbitrary length arguments - Linux
4f44cf6: 8341481: [perf] vframeStreamCommon constructor may be optimized
390b205: 8346048: test/lib/containers/docker/DockerRunOptions.java uses addJavaOpts() from ctor
03821d9: 8346195: Fix static initialization problem in GDIHashtable
a5503fb: 8346432: java.lang.foreign.Linker comment typo
... and 131 more: https://git.openjdk.org/jdk/compare/69e664de14d1f9d66447937d494da8bf971ac5fe...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-18T06:35:43Z

@sendaoYan Pushed as commit 842f801.

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

8345758: AArch64: vm_version_linux_aarch64.cpp report nullptr compile…

cf19466

… warning by gcc14

openjdk bot added the rfr Pull request is ready for review label Dec 10, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 10, 2024

sendaoYan changed the title ~~8345758: AArch64: vm_version_linux_aarch64.cpp report nullptr compile warning by gcc14~~ 8339331: GCC fortify error in vm_version_linux_aarch64.cpp Dec 10, 2024

theRealAph reviewed Dec 10, 2024

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp Outdated Show resolved Hide resolved

Use guarantee instead of assert to suppress gcc warnings

110539b

dholmes-ora approved these changes Dec 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 17, 2024

kimbarrett suggested changes Dec 17, 2024

View reviewed changes

Use PRAGMA_NONNULL_IGNORED to suppress gcc compile warning, and add c…

f31bd04

…omments about the gcc bug

openjdk bot removed the ready Pull request is ready to be integrated label Dec 17, 2024

revert to "invalid argument"

5b4e28c

kimbarrett suggested changes Dec 17, 2024

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp Outdated Show resolved Hide resolved

update comments

4e30aca

dholmes-ora approved these changes Dec 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 18, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 18, 2024

openjdk bot closed this Dec 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 18, 2024

sendaoYan deleted the jbs8345758 branch December 18, 2024 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8339331: GCC fortify error in vm_version_linux_aarch64.cpp #22655

8339331: GCC fortify error in vm_version_linux_aarch64.cpp #22655

sendaoYan commented Dec 10, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 10, 2024

openjdk bot commented Dec 10, 2024 •

edited

Loading

openjdk bot commented Dec 10, 2024

mlbridge bot commented Dec 10, 2024 •

edited

Loading

sendaoYan commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

sendaoYan commented Dec 10, 2024

dholmes-ora left a comment

kimbarrett left a comment

sendaoYan commented Dec 17, 2024

theRealAph commented Dec 17, 2024

dholmes-ora left a comment

sendaoYan commented Dec 18, 2024 •

edited

Loading

sendaoYan commented Dec 18, 2024

openjdk bot commented Dec 18, 2024

openjdk bot commented Dec 18, 2024

8339331: GCC fortify error in vm_version_linux_aarch64.cpp #22655

8339331: GCC fortify error in vm_version_linux_aarch64.cpp #22655

Conversation

sendaoYan commented Dec 10, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 10, 2024

openjdk bot commented Dec 10, 2024 • edited Loading

openjdk bot commented Dec 10, 2024

mlbridge bot commented Dec 10, 2024 • edited Loading

Webrevs

sendaoYan commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

sendaoYan commented Dec 10, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

sendaoYan commented Dec 17, 2024

theRealAph commented Dec 17, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

sendaoYan commented Dec 18, 2024 • edited Loading

sendaoYan commented Dec 18, 2024

openjdk bot commented Dec 18, 2024

openjdk bot commented Dec 18, 2024

sendaoYan commented Dec 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 10, 2024 •

edited

Loading

mlbridge bot commented Dec 10, 2024 •

edited

Loading

sendaoYan commented Dec 18, 2024 •

edited

Loading