8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code #20385

kimbarrett · 2024-07-30T04:12:33Z

Please review this change that removes some uses of literal 0 as a null
pointer constant in prims code.

Testing: mach5 tier1

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-8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code (Enhancement - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer)
Julian Waters (@TheShermanTanker - Committer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20385

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-30T04:13:25Z

👋 Welcome back kbarrett! 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-07-30T04:14:22Z

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

8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code

Reviewed-by: dholmes, shade, jwaters, sspitsyn

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

de0b504: 8336912: G1: Undefined behavior for G1ConfidencePercent=0
e63d016: 8337031: Improvements to CompilationMemoryStatistic
1c6fef8: 8337515: JVM_DumpAllStacks is dead code
5b7bb40: 8325002: Exceptions::fthrow needs to ensure it truncates to a valid utf8 string
d39e7af: 8320561: Inconsistency in monitorinflation logging
6c3ba5a: 8337415: Remove inappropriate Atomic access in FreeListAllocator
f5c9e8f: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID
93c19ac: 8337219: AccessFlags factories do not require necessary arguments
6154a21: 8336032: Enforce immutability of Lists used by ClassFile API
a91f9ba: 8301403: Eliminate memory allocations in JVMFlag::printFlags during signal handling
... and 10 more: https://git.openjdk.org/jdk/compare/7e21d4c1916d6690b717911179314c26a0da5ed9...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-07-30T04:14:56Z

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

hotspot
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.

mlbridge · 2024-07-30T04:18:02Z

Webrevs

00: Full (fc89216c)

dholmes-ora

Couple of queries on this one.

Thanks

dholmes-ora · 2024-07-30T05:27:37Z

src/hotspot/share/prims/jni.cpp

-  ResultType ret = 0;\
+  ResultType ret{}; \


This looks bogus. ResultType is just a macro variable and could be a primitive type. ?? Does the local need initializing?

This is value-initialization syntax. Value-initialization of a primitive type is zero-initialization.

However, I think we don't need the local variable at all. Here and in the other 5(?) similar places, rather than

ResultType ret{}; ... ret = jvalue.get_##ResultType(); return ret;

I think we could just have

... return jvalue.get_##ResultType();

Looks like eliminating the variable doesn't work. It gets used in a DT_RETURN_MARK_FOR form, which
needs the address of the return value. That address is obtained using a reference. Taking a reference
to an uninitialized variable is (I think) okay, so long as one doesn't attempt to use the uninitialized value.
But then the assignment could be problematic if it's uninitialized and the assignment operator is non-trivial.
I expect the compiler will optimize away a trivial zero initialization if it's not needed. So ensuring it is
value-initialized seems like the cleanest thing to do.

One day I will remember what this syntax is and does.

dholmes-ora · 2024-07-30T05:28:52Z

src/hotspot/share/prims/methodHandles.cpp

@@ -436,7 +436,6 @@ Symbol* MethodHandles::signature_polymorphic_intrinsic_name(vmIntrinsics::ID iid
  case vmIntrinsics::_linkToNative:     return vmSymbols::linkToNative_name();
  default:
    fatal("unexpected intrinsic id: %d %s", vmIntrinsics::as_int(iid), vmIntrinsics::name_at(iid));
-    return 0;


Do we no longer need these returns after fatal to keep compilers happy?

Now that we have, and are using, [[noreturn]] on all platforms, we no longer need that dead code.

I'll admit, I do prefer having a return to end all possible control flows in a non void method, but oh well

I would rather it not look like it can return null (or some other manufactured "default") when it actually can't.

dholmes-ora

Okay - looks good. Thanks.

shipilev

All right, this looks fine. (I am somewhat allergic to {} syntax, but it is what it is.)

TheShermanTanker

Looks Good!

kimbarrett · 2024-07-31T02:04:27Z

All right, this looks fine. (I am somewhat allergic to {} syntax, but it is what it is.)

The hoops one had to go through to get guaranteed value-initialization before we had brace initialization are really
not pretty. See
https://www.boost.org/doc/libs/1_85_0/libs/utility/doc/html/utility/utilities/value_init.html
and its associated implementation.

It might help if we were to commit to using direct brace initialization whenever appropriate, but that hasn't happened.

sspitsyn

Looks good. Thank you for fixing this!
The ResultType ret{}; syntax is a little bit unusual but I'm okay with that. :)

kimbarrett · 2024-07-31T13:12:13Z

Thanks for reviews @dholmes-ora , @shipilev , @TheShermanTanker , and @sspitsyn .

kimbarrett · 2024-07-31T13:14:19Z

/integrate

openjdk · 2024-07-31T13:15:53Z

Going to push as commit 07dd725.
Since your change was applied there have been 22 commits pushed to the master branch:

c73b3cb: 8336635: Add IR test for Reference.refersTo intrinsic
9b428dd: 8336242: compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/SimpleDebugInfoTest.java failed assert(oopDesc::is_oop_or_null(val)) failed: bad oop found (again)
de0b504: 8336912: G1: Undefined behavior for G1ConfidencePercent=0
e63d016: 8337031: Improvements to CompilationMemoryStatistic
1c6fef8: 8337515: JVM_DumpAllStacks is dead code
5b7bb40: 8325002: Exceptions::fthrow needs to ensure it truncates to a valid utf8 string
d39e7af: 8320561: Inconsistency in monitorinflation logging
6c3ba5a: 8337415: Remove inappropriate Atomic access in FreeListAllocator
f5c9e8f: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID
93c19ac: 8337219: AccessFlags factories do not require necessary arguments
... and 12 more: https://git.openjdk.org/jdk/compare/7e21d4c1916d6690b717911179314c26a0da5ed9...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-31T13:15:59Z

@kimbarrett Pushed as commit 07dd725.

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

fix warnings in prims

fc89216

openjdk bot added the rfr Pull request is ready for review label Jul 30, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jul 30, 2024

dholmes-ora reviewed Jul 30, 2024

View reviewed changes

dholmes-ora approved these changes Jul 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 30, 2024

shipilev approved these changes Jul 30, 2024

View reviewed changes

TheShermanTanker approved these changes Jul 30, 2024

View reviewed changes

sspitsyn approved these changes Jul 31, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 31, 2024

openjdk bot closed this Jul 31, 2024

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

kimbarrett deleted the simple-prims branch July 31, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code #20385

8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code #20385

kimbarrett commented Jul 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024 •

edited

Loading

openjdk bot commented Jul 30, 2024

mlbridge bot commented Jul 30, 2024

dholmes-ora left a comment

dholmes-ora Jul 30, 2024

kimbarrett Jul 30, 2024

kimbarrett Jul 30, 2024

dholmes-ora Jul 30, 2024

dholmes-ora Jul 30, 2024

kimbarrett Jul 30, 2024

TheShermanTanker Jul 30, 2024

kimbarrett Jul 31, 2024

dholmes-ora left a comment

shipilev left a comment

TheShermanTanker left a comment

kimbarrett commented Jul 31, 2024

sspitsyn left a comment

kimbarrett commented Jul 31, 2024

kimbarrett commented Jul 31, 2024

openjdk bot commented Jul 31, 2024

openjdk bot commented Jul 31, 2024

8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code #20385

8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code #20385

Conversation

kimbarrett commented Jul 30, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024 • edited Loading

openjdk bot commented Jul 30, 2024

mlbridge bot commented Jul 30, 2024

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

TheShermanTanker left a comment

Choose a reason for hiding this comment

kimbarrett commented Jul 31, 2024

sspitsyn left a comment

Choose a reason for hiding this comment

kimbarrett commented Jul 31, 2024

kimbarrett commented Jul 31, 2024

openjdk bot commented Jul 31, 2024

openjdk bot commented Jul 31, 2024

kimbarrett commented Jul 30, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 30, 2024 •

edited

Loading