On Thu, 30 Sep 2021 16:23:34 GMT, Yumin Qi <[email protected]> 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
