RFR: 8273152: Refactor CDS FileMapHeader loading code [v2]

[Calvin Cheung]

On Thu, 30 Sep 2021 16:23:34 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> Please review,
>>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic info from shared archive.
>>   With the change, it makes it possible to read an archive generated by different version of hotspot. Also it is possible to automatically generate a CDS archive If the archive supplied is not readable or fails to pass the check.
>> 
>>   Tests: tier1-4
>>    jtreg on sa.
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed miss return after file read failed

Looks good. Just a few minor comments.

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

> 281:   assert(base_archive_name_size() != 0, "_base_archive_name_size not set");
> 282:   assert(base_archive_path_offset() != 0, "_base_archive_path_offset not set");
> 283:   assert(header_size() > sizeof(*this), "_base_archive_name_size not included in head size?");

Typo: head -> header

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

> 1152:     // read the base archive name
> 1153:     size_t name_size = (size_t)header->_base_archive_name_size;
> 1154:     assert(name_size != 0, "None default base archive, name size should not be zero!");

Typo: None default -> Non-default

test/hotspot/jtreg/runtime/cds/appcds/SharedArchiveConsistency.java line 226:

> 224: 
> 225:         // modify _base_archive_path_offet to not zero
> 226:         System.out.println("\n8. modify _base_archive_path_offset to not zero\n");

Suggestion: not zero -> non-zero

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ArchiveConsistency.java line 109:

> 107: 
> 108:         // 2. Make header size biger than the archive size
> 109:         System.out.println("\n2. Make header size biger than the archive size");

Typo: biger -> larger

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ArchiveConsistency.java line 124:

> 122:         int fileHeaderSize = (int)CDSArchiveUtils.fileHeaderSize(copiedJsa);
> 123:         int baseArchivePathOffset = CDSArchiveUtils.baseArchivePathOffset(copiedJsa);
> 124:         CDSArchiveUtils.modifyHeaderIntField(copiedJsa, CDSArchiveUtils.offsetBaseArchivePathOffset, baseArchivePathOffset + 1024);

In line 124, could `baseArchivePathOffset` be used instead of `CDSArchiveUtils.offsetBaseArchivePathOffset` ?

test/lib/jdk/test/lib/cds/CDSArchiveUtils.java line 139:

> 137:         int size = baseArchiveNameSize(jsaFile);
> 138:         int baseArchivePathOffset = (int)readInt(jsaFile, offsetBaseArchivePathOffset, 4);
> 139:         return readString(jsaFile, baseArchivePathOffset, size - 1); // exclude including ending '\0'

Suggestion on the comment:
// exclude the terminating ‘\0’

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

Changes requested by ccheung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5768