8366699: Replace TestNoNULL with a simpler mechanism to forbid using NULL in HotSpot code

[TheShermanTanker]

TestNoNULL was introduced as a way to forbid using the raw NULL macro in HotSpot code. It works well, but a newer way to achieve the same result by poisoning the macro expansion itself would be more accurate given it could properly use the compiler's lexer and parser to detect NULL being introduced. Normally the way typically envisioned to poison macros by redefining them as an unusable sequence of tokens would be too strong and bleed over into third party code, but there is a way to do so in a controllable fashion. This proposes to implement a simple mechanism to replace TestNoNULL within HotSpot's source code itself, and retire TestNoNULL in favour of this new system.

For Visual C++ this is simple, it contains support for using a pragma to poison a macro name, and poisoning a macro causes warnings when it is used. These warnings can be switched off by pragmas whenever we need them to be, making for a perfect disabling system when NULL is used in third party code.

For the other 2 big compilers, there exists a similar pragma to disable using a macro, but it is too strong, and unlike Visual C++ is a compile error, meaning there is absolutely no way to turn it off whatsoever. Fortunately there is a simple trick one can leverage: Both compilers have a warning pragma that can be used in this case. All that has to be done is redefining the macro in question to expand into nullptr, but also expand into a pragma that contains this warning message. As for disabling it whenever third party code is around, both compilers conveniently have a push and pop macro pragma that can save and restore macro state, which they implemented for compatibility with Visual C++ and are handy for this purpose. Since redefining Standard header macros is not allowed in C++ we can instead target an implementation detail within the headers: NULL expands to __null, so we can define that as a macro for our purposes instead.

Currently using GitHub Actions to test the changes, this is not the final state of the Pull Request.

---

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-8366699: Replace TestNoNULL with a simpler mechanism to forbid using NULL in HotSpot code (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27054

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jwaters! 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]

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

[openjdk]

@TheShermanTanker To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

• build
• client
• compiler
• core-libs
• graal
• hotspot
• hotspot-compiler
• hotspot-gc
• hotspot-jfr
• hotspot-runtime
• i18n
• ide-support
• javadoc
• jdk
• jmx
• net
• nio
• security
• serviceability
• shenandoah

[TheShermanTanker]

/label hotspot

[openjdk]

@TheShermanTanker
The hotspot label was successfully added.

[mlbridge]

Webrevs

• 00: Full (fc772b76)

[kimbarrett]

Sorry, but this feels like a heavy-weight solution to an almost non-problem.
I agree that it would be nice to detect uses of NULL earlier than we do now.
But I don't feel like that gain is worth this change.

And the cost from this change may not be negligible. I've been experimenting
with some changes involving use of some parts of the C++ standard library, and
already need to wrap inclusions to deal with HotSpot's assert macro and
function forbidding. This would add another thing that needs to be turned off.
(Perhaps ameliorated by introducing BEGIN/END_THIRD_PARTY_CODE macros or
something like that.)

I'm also surprised this didn't get tripped by gtests. googletest does use NULL
in some places. (You did include that in your testing, right?)

This proposal also doesn't address use of NULL in comments and string literals.
We decided we also wanted to disallow those, for consistency with usage in the
code. (It also made the checking easier.)

[kimbarrett]

This change seems unrelated to the topic at hand, and I'm unconvinced it's even good.
Being explicit through naming that the option is a string seems beneficial to me.

[kimbarrett]

I didn't notice until after commenting that this PR was demoted to draft, possibly because of the test failures
on Windows. But my disagreement with making this change likely stands regardless.

[TheShermanTanker]

I didn't notice until after commenting that this PR was demoted to draft, possibly because of the test failures on Windows. But my disagreement with making this change likely stands regardless.

Looks like Skara went ahead and sent the Pull Request to the mailing list despite this being marked as a draft. I was still working on it. Sigh. I didn't want this to be seen until it was perfectly ready.

[TheShermanTanker]

I'm also surprised this didn't get tripped by gtests. googletest does use NULL
in some places. (You did include that in your testing, right?)

Yes, it is included. Seems like the relevant headers that are included by the main headers that are included don't use NULL. I would be surprised if they suddenly stopped working with this change

This proposal also doesn't address use of NULL in comments and string literals.
We decided we also wanted to disallow those, for consistency with usage in the
code. (It also made the checking easier.)

I was not aware that NULL is also not allowed in comments now (I thought it was just the macro). There really is no way to check for NULL in comments in just C++ alone. This proposal is now non-implementable, I'll close it shortly

And the cost from this change may not be negligible. I've been experimenting
with some changes involving use of some parts of the C++ standard library, and
already need to wrap inclusions to deal with HotSpot's assert macro and
function forbidding. This would add another thing that needs to be turned off.
(Perhaps ameliorated by introducing BEGIN/END_THIRD_PARTY_CODE macros or
something like that.)

I was under the impression that most of it was forbidden from being used, I guess that's changing now? The main concern would seem to be the poisoned macro causing issues in Standard Library headers, at least to me. That's good that more facilities are starting to be allowed, though

Seems like I'm back to the drawing board. Perhaps that's all the more reason to properly commit to writing that static analyzer I had planned for HotSpot that I stopped working on halfway through due to other commitments, or alternatively resume trying to implement features in the upstream compilers that can help with these checks, at least in the ones that are open source. We recently got static analyzer support for gcc, that's a perfect opportunity to use that to enforce HotSpot rules if my efforts are successful

[kimbarrett]

I was not aware that NULL is also not allowed in comments now (I thought it was just the macro). There really is no way to check for NULL in comments in just C++ alone. This proposal is now non-implementable, I'll close it shortly

And the cost from this change may not be negligible. I've been experimenting
with some changes involving use of some parts of the C++ standard library, [...]

I was under the impression that most of it was forbidden from being used, I guess that's changing now?

It's not changing yet. I've been working off and on to change that, doing
experiments and working on arguments that might convince some folks.

I would like us to be able to use standard containers and algorithms, for
example. I think GrowableArray is pretty lame (and there are others who are
unhappy with its limitations too), but std::vector has some assumptions and
choices that might not be right for us. A bunch of time was recently spent
adding a red/black tree utility, where I think it would be better if we could
just use std::map/set. Memory management is one of the sticky points. And
while C++ has gotten better, there are still some serious worms there.

I think it might be easier to get some other smaller pieces into use, and will
likely be working toward that after C++17 acceptance.

And to be clear, this might end up not going anywhere. There are some very
strong conservative opinions in the community, and I even think at least some
of that is justified.

[TheShermanTanker]

Alright, thanks. Will keep the linked issue open and repurpose it for the static analyzer, and just close this Pull Request only