On Thu, 30 Sep 2021 16:23:34 GMT, Yumin Qi <mi...@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