8342860: Fix more NULL usage backsliding #21826

theoweidmannoracle · 2024-11-01T13:35:56Z

Changed several "NULL" in comments to "null"
Changed several NULL in code to nullptr

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-8342860: Fix more NULL usage backsliding (Enhancement - P4)

Reviewers

Johan Sjölen (@jdksjolen - Reviewer) Review applies to 9754145b
Kim Barrett (@kimbarrett - Reviewer)
Julian Waters (@TheShermanTanker - Committer)
Thomas Schatzl (@tschatzl - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21826

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-01T13:36:38Z

Hi @theoweidmannoracle, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user theoweidmannoracle" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2024-11-01T13:36:58Z

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

8342860: Fix more NULL usage backsliding

Reviewed-by: kbarrett, jwaters, tschatzl, jsjolen

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

f6edfe5: 8343506: [s390x] multiple test failures with ubsan
96eed7f: 8343306: javac is failing to determine if a class and a sealed interface are disjoint
0c281ac: 8343754: Problemlist jdk/jfr/event/oldobject/TestShenandoah.java after JDK-8279016
2e58ede: 8341399: Add since checker tests to the langtools modules
c7f071c: 8343189: [REDO] JDK-8295269 G1: Improve slow startup due to predictor initialization
a9e53bb: 8343783: Improve asserts in concurrentHashTable.inline.hpp
bf5c3ce: 8272780: ServerNotifForwarder.removeNotificationListener() incorrect exception handling
a10b1cc: 8340532: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue
d0077ee: 8343771: Some FFM benchmarks are broken
068f4ce: 8343293: Remove the check for /jre/lib/libjava.dylib from the launcher's java_md_macosx.m
... and 51 more: https://git.openjdk.org/jdk/compare/c33a8f52b613e5eff02f572eda876cbbfc7c22cf...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@SoniaZaldana, @jdksjolen, @kimbarrett, @TheShermanTanker, @tschatzl) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-11-01T13:37:29Z

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

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

theoweidmannoracle · 2024-11-01T13:38:09Z

/covered Oracle Employee

SoniaZaldana

I am not a Reviewer but I left a small comment.

Cheers!

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

mlbridge · 2024-11-05T13:19:08Z

Webrevs

SoniaZaldana · 2024-11-05T14:22:06Z

Hi @theoweidmannoracle, I think the GHA tests are not running because you haven't enabled GHA on your personal fork. See https://wiki.openjdk.org/display/SKARA/Testing for a bit more info.

theoweidmannoracle · 2024-11-05T14:49:22Z

Hi @SoniaZaldana, thanks for pointing that out. I enabled the tests now, but it seems there's no way to run them except for pushing new changes. I did run Oracle's internal testing, though.

jdksjolen

Thank you, these changes looks good to me.

SoniaZaldana · 2024-11-05T15:00:37Z

Hi @SoniaZaldana, thanks for pointing that out. I enabled the tests now, but it seems there's no way to run them except for pushing new changes. I did run Oracle's internal testing, though.

You can try syncing your fork and that should trigger GHA.

theoweidmannoracle · 2024-11-05T15:03:26Z

@SoniaZaldana Thanks for the tip!

kimbarrett

Can you use the (updated) regex in the JBS issue description to verify the only
remaining "NULL"s in src/hotspot are jvmti.{xml,xsl} and
globalDefinitons_{gcc,visCPP}.hpp?

There are also some new NULLs in test/hotspot.
./jtreg/serviceability/jvmti/GetMethodDeclaringClass/libTestUnloadedClass.cpp
./jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp
There are a couple more (after filtering out java and C source files) that I think
shouldn't be changed.

kimbarrett

Can you use the (updated) regex in the JBS issue description to verify the
only remaining "NULL"s in src/hotspot are the jvmti.{xml,xls} files and the
globalDefinitions_{gcc,visCPP}.hpp files?

There are also some NULLs recently introduced in test/hotspot:
./jtreg/serviceability/jvmti/GetMethodDeclaringClass/libTestUnloadedClass.cpp
./jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp

(Found by applying the same regex to test/hotspot, and then removing .java and
.c files.)

There are a few other files in test/hotspot containing NULLs:
./jtreg/vmTestbase/nsk/share/jni/README
./jtreg/vmTestbase/nsk/share/jvmti/README
These are documentation files with examples written in C, so should not be changed.

./jtreg/vmTestbase/nsk/share/native/nsk_tools.hpp
In a comment describing a string to be used for printing. Uses would need to
be examined to ensure it's okay to change the string used for a null value.
I think I planned to do this as a followup to JDK-8324799, and then forgot.
I'd be okay with doing something about this being separate from the current
PR. While the necessary textual changes are probably small, there's a lot of
uses to examine to be sure a change is okay.

kimbarrett · 2024-11-05T16:13:15Z

Hi @SoniaZaldana, thanks for pointing that out. I enabled the tests now, but it seems there's no way to run them except for pushing new changes. I did run Oracle's internal testing, though.

To manually trigger GHA tests:

Go to your personal fork, and click on the "Actions" menu item.
Select the "OpenJDK GHA Sanity Checks" Action.
Click on the "Run workflow" pulldown.
Select the branch you want to test in the "Use workflow from" pulldown.
Click on "Run workflow".

theoweidmannoracle · 2024-11-07T08:28:46Z

Can you use the (updated) regex in the JBS issue description to verify the only remaining "NULL"s in src/hotspot are the jvmti.{xml,xls} files and the globalDefinitions_{gcc,visCPP}.hpp files?

There are also some NULLs recently introduced in test/hotspot: ./jtreg/serviceability/jvmti/GetMethodDeclaringClass/libTestUnloadedClass.cpp ./jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp

(Found by applying the same regex to test/hotspot, and then removing .java and .c files.)

There are a few other files in test/hotspot containing NULLs: ./jtreg/vmTestbase/nsk/share/jni/README ./jtreg/vmTestbase/nsk/share/jvmti/README These are documentation files with examples written in C, so should not be changed.

./jtreg/vmTestbase/nsk/share/native/nsk_tools.hpp In a comment describing a string to be used for printing. Uses would need to be examined to ensure it's okay to change the string used for a null value. I think I planned to do this as a followup to JDK-8324799, and then forgot. I'd be okay with doing something about this being separate from the current PR. While the necessary textual changes are probably small, there's a lot of uses to examine to be sure a change is okay.

@kimbarrett I fixed the backslides in the *.cpp files you mentioned. The egrep outputs are now:

% find test/hotspot -type f ! -name "*.c" ! -name "*.java" -exec egrep -H "[^[:alnum:]_]NULL([^[:alnum:]_]|$)" {} \;
test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.hpp: * Returns str or "<NULL>" if str is null; useful for printing strings.
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/README:    if (!NSK_JVMTI_VERIFY(jvmti->GetVersion(&version) != NULL)) {
test/hotspot/jtreg/vmTestbase/nsk/share/jni/README:            jni->FindClass(class_name) != NULL)) {
test/hotspot/jtreg/vmTestbase/nsk/share/jni/README:            jni->FindClass(class_name)) != NULL)) {

and

egrep -R "[^[:alnum:]_]NULL([^[:alnum:]_]|$)" src/hotspot                                                      
src/hotspot/share/prims/jvmti.xml:    or return value. A "null pointer" is C <code>NULL</code> or C++ <code>nullptr</code>.
src/hotspot/share/prims/jvmti.xml:                       &amp;methodName, NULL, NULL);
src/hotspot/share/prims/jvmti.xml:  GetThreadCpuTime(env, NULL, nanos_ptr)
src/hotspot/share/prims/jvmti.xsl:        <xsl:value-of select="@const"/>, NULL)</code>
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// When __cplusplus is defined, NULL is defined as 0 (32-bit constant) in
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// On 64-bit architectures, defining NULL as a 32-bit constant can cause
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// varargs, we pass the argument 0 as an int.  So, if NULL was passed to a
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// only 32-bits of the "NULL" pointer may be initialized to zero.  The
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// Solution: For 64-bit architectures, redefine NULL as 64-bit constant 0.
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:#undef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:#define NULL 0LL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:#ifndef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:#define NULL 0
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// NULL vs NULL_WORD:
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:// On Linux NULL is defined as a special type '__null'. Assigning __null to
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:#define NULL_WORD NULL
src/hotspot/share/utilities/globalDefinitions_gcc.hpp:// NULL vs NULL_WORD:
src/hotspot/share/utilities/globalDefinitions_gcc.hpp:// On Linux NULL is defined as a special type '__null'. Assigning __null to
src/hotspot/share/utilities/globalDefinitions_gcc.hpp:  #define NULL_WORD  NULL

kimbarrett

Looks good.

The grep results are exactly as expected. Thanks for checking that. Now if we can
just get the build to start checking for us, we can stop needing these periodic cleanups.
I forget whether a JBS issue has been filed for that. If not, I will do so.

kimbarrett · 2024-11-07T21:46:16Z

The grep results are exactly as expected. Thanks for checking that. Now if we can just get the build to start checking for us, we can stop needing these periodic cleanups. I forget whether a JBS issue has been filed for that. If not, I will do so.

II've filed these followup issues:
https://bugs.openjdk.org/browse/JDK-8343802 Prevent NULL usage backsliding
https://bugs.openjdk.org/browse/JDK-8343800 Cleanup definition of NULL_WORD
https://bugs.openjdk.org/browse/JDK-8343801 Change string printed by nsk_null_string for null strings

theoweidmannoracle · 2024-11-08T10:05:38Z

@jdksjolen @TheShermanTanker It would be great if you could also take another look at the latest changes I made. Thanks!

theoweidmannoracle · 2024-11-08T12:53:21Z

/integrate

openjdk · 2024-11-08T12:54:01Z

@theoweidmannoracle
Your change (at version e79b7bd) is now ready to be sponsored by a Committer.

TheShermanTanker · 2024-11-08T13:30:54Z

/sponsor

openjdk · 2024-11-08T13:31:25Z

Going to push as commit 7d6a2f3.
Since your change was applied there have been 61 commits pushed to the master branch:

f6edfe5: 8343506: [s390x] multiple test failures with ubsan
96eed7f: 8343306: javac is failing to determine if a class and a sealed interface are disjoint
0c281ac: 8343754: Problemlist jdk/jfr/event/oldobject/TestShenandoah.java after JDK-8279016
2e58ede: 8341399: Add since checker tests to the langtools modules
c7f071c: 8343189: [REDO] JDK-8295269 G1: Improve slow startup due to predictor initialization
a9e53bb: 8343783: Improve asserts in concurrentHashTable.inline.hpp
bf5c3ce: 8272780: ServerNotifForwarder.removeNotificationListener() incorrect exception handling
a10b1cc: 8340532: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue
d0077ee: 8343771: Some FFM benchmarks are broken
068f4ce: 8343293: Remove the check for /jre/lib/libjava.dylib from the launcher's java_md_macosx.m
... and 51 more: https://git.openjdk.org/jdk/compare/c33a8f52b613e5eff02f572eda876cbbfc7c22cf...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-08T13:31:34Z

@TheShermanTanker @theoweidmannoracle Pushed as commit 7d6a2f3.

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

8342860: Fix more NULL usage backsliding

baa31f7

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 1, 2024

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 1, 2024

SoniaZaldana suggested changes Nov 4, 2024

View reviewed changes

src/hotspot/os/linux/cgroupSubsystem_linux.cpp Show resolved Hide resolved

Fix copyright year

afb592f

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Nov 5, 2024

openjdk bot added the rfr Pull request is ready for review label Nov 5, 2024

jdksjolen approved these changes Nov 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 5, 2024

Merge branch 'openjdk:master' into JDK-8342860

9754145

kimbarrett suggested changes Nov 5, 2024

View reviewed changes

TheShermanTanker approved these changes Nov 5, 2024

View reviewed changes

kimbarrett suggested changes Nov 5, 2024

View reviewed changes

Fix backslides in test

e79b7bd

openjdk bot removed the ready Pull request is ready to be integrated label Nov 7, 2024

kimbarrett approved these changes Nov 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 7, 2024

TheShermanTanker approved these changes Nov 8, 2024

View reviewed changes

tschatzl approved these changes Nov 8, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 8, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 8, 2024

openjdk bot closed this Nov 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 8, 2024

theoweidmannoracle deleted the JDK-8342860 branch November 8, 2024 13:32

8342860: Fix more NULL usage backsliding #21826

8342860: Fix more NULL usage backsliding #21826

Uh oh!

Conversation

theoweidmannoracle commented Nov 1, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 1, 2024

Uh oh!

openjdk bot commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 1, 2024

Uh oh!

theoweidmannoracle commented Nov 1, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SoniaZaldana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

SoniaZaldana commented Nov 5, 2024

Uh oh!

theoweidmannoracle commented Nov 5, 2024

Uh oh!

jdksjolen left a comment

Choose a reason for hiding this comment

Uh oh!

SoniaZaldana commented Nov 5, 2024

Uh oh!

theoweidmannoracle commented Nov 5, 2024

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett left a comment • 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!

kimbarrett commented Nov 5, 2024

Uh oh!

theoweidmannoracle commented Nov 7, 2024

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett commented Nov 7, 2024

Uh oh!

theoweidmannoracle commented Nov 8, 2024

Uh oh!

theoweidmannoracle commented Nov 8, 2024

Uh oh!

openjdk bot commented Nov 8, 2024

Uh oh!

TheShermanTanker commented Nov 8, 2024

Uh oh!

openjdk bot commented Nov 8, 2024

Uh oh!

openjdk bot commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

theoweidmannoracle commented Nov 1, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 1, 2024 •

edited

Loading

theoweidmannoracle commented Nov 1, 2024 •

edited by bridgekeeper bot

Loading

mlbridge bot commented Nov 5, 2024 •

edited

Loading

kimbarrett left a comment •

edited

Loading