RFR: 8321183: Incorrect warning from cds about the modules file

[Calvin Cheung]

On Fri, 1 Dec 2023 15:52:11 GMT, Maxim Kartashev <mkartashev at openjdk.org> wrote:

> When the `modules`  file used at run time differs in size from the one used at build time, the warning is confusing:
> 
> 
> $ .../bin/java -jar .../demo/jfc/Notepad/Notepad.jar
> [0.033s][warning][cds] A jar file is not the one used while building the shared archive file: /.../lib/modules
> [0.033s][warning][cds] A jar file is not the one used while building the shared archive file: /.../lib/modules
> [0.033s][warning][cds] /.../lib/modules timestamp has changed. 
> 
> Notice that it is referred to as `jar file` and that `timestamp has changed` rather than size.
> 
> The suggested patch is pretty self-explanatory. With this patch, the warnings look like this:
> 
> ./build/linux-x86_64-server-release/images/jdk/bin/java -jar ./build/linux-x86_64-server-release/images/jdk/demo/jfc/Notepad/Notepad.jar
> [0.014s][warning][cds] This file is not the one used while building the shared archive file: /home/maxim/openjdk/jdk/build/linux-x86_64-server-release/images/jdk/lib/modules
> [0.014s][warning][cds] This file is not the one used while building the shared archive file: /home/maxim/openjdk/jdk/build/linux-x86_64-server-release/images/jdk/lib/modules
> [0.014s][warning][cds] /home/maxim/openjdk/jdk/build/linux-x86_64-server-release/images/jdk/lib/modules size has changed.

If I understand the bug correct, is the original code at line 413 missing a condition?
`if (_timestamp != st.st_mtime) {` 
should be 
`if (has_timestamp() && (_timestamp != st.st_mtime)) {`

>From your reproducing steps in the bug report:
"3. replace the .../lib/modules file in the first with the corresponding file from the second, making sure they differ in size"
After replacing the file, would the new file have a different timestamp?

Since you've changed the log statement from "A jar file..." to "This file...", the following tests under the appcds directory need to be adjusted:
WrongClasspath.java
dynamicArchive/MainModuleOnly.java
dynamicArchive/WrongTopClasspath.java
jigsaw/modulepath/MainModuleOnly.java

One suggestion below.

src/hotspot/share/cds/filemap.cpp line 416:

> 414:           log_warning(cds)("%s timestamp has changed.", name);
> 415:         } else {
> 416:           log_warning(cds)("%s size has changed.", name);

Instead of `else`, maybe you can do the following because a file could differ both in timestamp and size.
         if (time_differs) {
           log_warning(cds)("%s timestamp has changed.", name);
         } 
         if (size_differs) {
           log_warning(cds)("%s size has changed.", name);
         }

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

Changes requested by ccheung (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16926#pullrequestreview-1760643367
PR Review Comment: https://git.openjdk.org/jdk/pull/16926#discussion_r1412621780