RFR: 8343802: Prevent NULL usage backsliding

[Kim Barrett]

On Wed, 5 Feb 2025 15:39:32 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

> Please review this patch to add a test that checks the hotspot sources and test files for usages of NULL.
> It scans files in those directories, filtering out certain files as well as all `.c`, `.java` and `.jar` files in test sources. 
> 
> Before adding line 86 and excluding `os_windows.cpp`, the test failed with:
> 
> 
> Error: 'NULL' found in /w/jdk/src/hotspot/os/windows/os_windows.cpp at line 4436:
>     HMODULE hModule = NULL;
> Error: 'NULL' found in /w/jdk/src/hotspot/os/windows/os_windows.cpp at line 4437:
>     GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, NULL, &hModule);
> java.lang.RuntimeException: Found usage of 'NULL' in source files. See errors above.
> 	at TestNoNULL.main(TestNoNULL.java:73)
> 	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
> 	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
> 	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
> 	at java.base/java.lang.Thread.run(Thread.java:1447)

Changes requested by kbarrett (Reviewer).

test/hotspot/jtreg/TEST.groups line 47:

> 45:   gc
> 46: 
> 47: hotspot_null_check = \

Where is this used?

test/hotspot/jtreg/TEST.groups line 228:

> 226:   -compiler/loopopts/Test7052494.java \
> 227:   -compiler/runtime/Test6826736.java \
> 228:   sources

This seems weirdly placed.  And I think we want the NULL check to be done in tier1.

test/hotspot/jtreg/sources/TestNoNULL.java line 47:

> 45:     private static final Set<String> excludedTestFiles = new HashSet<>();
> 46:     private static final Set<String> excludedTestExtensions = Set.of(".c", ".java", ".jar");
> 47:     private static final Pattern NULL_PATTERN = Pattern.compile("\\bNULL\\b");

I don't think this is the right pattern to use.  See the description in JDK-8343802 for the pattern I think
should be used.

test/hotspot/jtreg/sources/TestNoNULL.java line 85:

> 83:                 "src/hotspot/share/prims/jvmti.xsl",
> 84:                 "src/hotspot/share/utilities/globalDefinitions_visCPP.hpp",
> 85:                 "src/hotspot/share/utilities/globalDefinitions_gcc.hpp",

globalDefinitions_gcc.hpp no longer needs to be excluded, since JDK-8343800 has been fixed.

test/hotspot/jtreg/sources/TestNoNULL.java line 103:

> 101:             public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
> 102:                 if (isIncluded(file, excludedFiles, excludeExtensions)) {
> 103:                     files.add(file);

Why collect files to be checked, rather than just checking them here?

test/hotspot/jtreg/sources/TestNoNULL.java line 135:

> 133:     private static boolean checkForNull(Path path) throws IOException {
> 134:         boolean found = false;
> 135:         List<String> lines = Files.readAllLines(path, StandardCharsets.UTF_8);

I would have thought it would be better to read and check a line at a time.  Though it probably doesn't really
matter all that much.

-------------

PR Review: https://git.openjdk.org/jdk/pull/23466#pullrequestreview-2596672636
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943484063
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943485458
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943489367
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943482428
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943522447
PR Review Comment: https://git.openjdk.org/jdk/pull/23466#discussion_r1943492846