[14] RFR(M): 8211723: AppCDS: referring to a jar file in any way other than exactly how it was done during dumping doesn't work
Jiangli Zhou
jianglizhou at google.com
Thu Jun 27 21:00:01 UTC 2019
Hi Calvin,
The updated change looks good. It's already much cleaner without the
other cleanups included. Those can be done afterwards.
Thanks and best regards,
Jiangli
On Thu, Jun 27, 2019 at 11:57 AM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>
>
>
> On 6/26/19, 5:38 PM, Jiangli Zhou wrote:
> > Hi Calvin,
> >
> > On Wed, Jun 26, 2019 at 3:49 PM Calvin Cheung<calvin.cheung at oracle.com> wrote:
> >> Hi Jiangli,
> >>
> >> Thanks for your review!
> >>
> >> On 6/26/19, 11:14 AM, Jiangli Zhou wrote:
> >>> Hi Calvin,
> >>>
> >>> Thank you for working on this. Really glad to see this finally being
> >>> done! I meant to review this earlier, but was delayed. Thank you for
> >>> being patient.
> >>>
> >>> - src/hotspot/share/classfile/classLoader.hpp
> >>>
> >>> 960 bool ClassLoader::update_class_path_entry_list(const char *path,
> >>> 961 bool check_for_duplicates,
> >>> 962 bool is_boot_append,
> >>> 963 bool from_class_path_attr,
> >>> 964 bool throw_exception) {
> >>> ...
> >>>
> >>> 984 #if INCLUDE_CDS
> >>> 985 if (DumpSharedSpaces || DynamicDumpSharedSpaces) {
> >>> 986 _shared_paths_misc_info->add_nonexist_path(path);
> >>> 987 }
> >>> 988 #endif
> >>>
> >>> The above code for _shared_paths_misc_info can be eliminated now. If a
> >>> path does not exist at dump time but exists at runtime, it should be
> >>> caught by the runtime shared path table check. I think the new check
> >>> you added for the shared path table does cover that case. We should
> >>> make sure we have sufficient test cases for that. It's good to also
> >>> add comments to the shared path table check to describe all cases.
> >> I don't think we can remove the above code because it will also update
> >> the class path entry list for the jar files which are found from the
> >> "Class-Path" attribute of a jar manifest.
> >>
> >> Below is the various call paths to the function (note the call from
> >> process_jar_manifest):
> >>
> >> ClassLoader::update_class_path_entry_list(const char *, bool, bool,
> >> bool, bool) : bool
> >> ClassLoader::setup_app_search_path(const char *) : void
> >> ClassLoader::setup_boot_search_path(const char *) : void
> >> ClassLoaderExt::process_jar_manifest(ClassPathEntry *, bool) : void
> >> ClassLoader::add_to_app_classpath_entries(const char *,
> >> ClassPathEntry *, bool) : void
> >> ClassLoader::update_class_path_entry_list(const char *,
> >> bool, bool, bool, bool) : bool
> > I was referring to the #if INCLUDED_CDS code for
> > _shared_paths_misc_info. It's no longer needed.
> >
> > 984 #if INCLUDE_CDS
> > 985 if (DumpSharedSpaces || DynamicDumpSharedSpaces) {
> > 986 _shared_paths_misc_info->add_nonexist_path(path);
> > 987 }
> > 988 #endif
> >
> > The rest of ClassLoader::update_class_path_entry_list() is still needed.
> Without the above code, it will fail with the following sceanrio:
> non-existing jar during dump time but jar exists during runtime
>
> I've added 2 test scenarios in the AppendClasspath.java test.
>
> Updated webrevs:
> incremental: http://cr.openjdk.java.net/~ccheung/8211723/delta_01_02/
> full: http://cr.openjdk.java.net/~ccheung/8211723/webrev.02/
> >
> >>>
> >>> - src/hotspot/share/memory/filemap.hpp
> >>>
> >>> 63 void init(const char* name, bool is_modules_image,
> >>> ClassPathEntry* cpe, TRAPS);
> >>>
> >>> The 'name' argument can be removed since it can be retrieved from
> >>> 'cpe', as you are passing the ClassPathEntry now.
> >> I've made the change.
> >>> - src/hotspot/share/classfile/sharedPathsMiscInfo.cpp
> >>>
> >>> With the removal of the problematic file name string comparison, the
> >>> SharedPathsMiscInfo::check() becomes basically a no-op now. It should
> >>> be removed now.
> >>>
> >>> 156 bool SharedPathsMiscInfo::check(jint type, const char* path, bool
> >>> is_static) {
> >>> 157 assert(UseSharedSpaces, "runtime only");
> >>> 158 switch (type) {
> >>> 159 case BOOT_PATH:
> >>> 160 break;
> >>> 161 case NON_EXIST:
> >>> 162 {
> >>> 163 struct stat st;
> >>> 164 if (os::stat(path,&st) == 0) {
> >>> 165 // The file actually exists
> >>> 166 // But we want it to not exist -> fail
> >>> 167 return fail("File must not exist");
> >>> 168 }
> >>> 169 }
> >>> 170 break;
> >>> 171 case APP_PATH:
> >>> 172 break;
> >>> 173 default:
> >>> 174 return fail("Corrupted archive file header");
> >>> 175 }
> >>> 176
> >>> 177 return true;
> >>> 178 }
> >>>
> >>> The following function should also be removed:
> >>> 107 bool SharedPathsMiscInfo::check(bool is_static) {
> >> The above functions can't be removed yet due to the same reasoning as
> >> above. It ensures the non-existing of the jar file(s) found in the
> >> "Class-Path" attributes.
> > I think we can enhance your current change to record the
> > 'non-existing' manifest Class-Path header referenced JAR files in the
> > shared path table at dump time. An entry can be added to the table and
> > flagged as 'non-existing' if one is such a case. At runtime, you can
> > check for that then. Please let me know if it's a bit vague, I can add
> > more details.
> As per our off-list discussion, this could be done in a follow-up RFE.
> I'll ask you for more details when I file the RFE.
>
> thanks,
> Calvin
> >
> > Best regards,
> > Jiangli
> >> Call paths to the function:
> >>
> >> SharedPathsMiscInfo::check(jint, const char *, bool) : bool
> >> SharedPathsMiscInfo::check(bool) : bool
> >> ClassLoader::check_shared_paths_misc_info(void *, int, bool) : bool
> >> FileMapInfo::validate_header(bool) : bool
> >> FileMapInfo::initialize(bool) : bool
> >> DynamicArchive::map_impl(FileMapInfo *) : address
> >>
> >> MetaspaceShared::initialize_runtime_shared_and_meta_spaces() : void
> >>> There are quite some additional SharedPathsMiscInfo cleanup and
> >>> removal needed. If you want to do those in separate changeset, that is
> >>> also okay. The cleanups will help make the CDS code more maintainable.
> >> I agree that cleaning up SharedPathsMiscInfo is possible but more
> >> thinking is required. So it should be done as a separate RFE/changeset.
> >>
> >>> - src/hotspot/os/posix/os_posix.cpp
> >>>
> >>> 1463 ret1 = os::stat(file1,&st1);
> >>> 1464 ret2 = os::stat(file2,&st2);
> >>> 1465 if (ret1< 0 || ret2< 0) {
> >>> 1466 // one of the files is invalid. So they are not the same.
> >>> 1467 is_same = false;
> >>> 1468 } else if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) {
> >>> 1469 // different files
> >>> 1470 is_same = false;
> >>> 1471 } else if (st1.st_dev == st2.st_dev&& st1.st_ino == st2.st_ino) {
> >>> 1472 // same files
> >>> 1473 is_same = true;
> >>> 1474 }
> >>> 1475 return is_same;
> >>>
> >>> The 'is_same' is initialized to false, so the cases where the files
> >>> are different do not need to be checked explicitly. We just need to
> >>> make sure 'is_same' is set to true for the case where the two files
> >>> are the same. So you can simplify and optimize the above a bit:
> >>>
> >>> if (os::stat(file1,&st1) != 0) {
> >>> return false;
> >>> }
> >>> if (os::stat(file2,&st2) != 0) {
> >>> return false;
> >>> }
> >>> if (st1.st_dev == st2.st_dev&& st1.st_ino == st2.st_ino) {
> >>> is_same = true;
> >>> }
> >>> return is_same;
> >> I've made the above change but checking the return status of os::stat as
> >> what I had before because ::stat returns -1 on failure:
> >> if (os::stat(file1,&st1)< 0) {
> >> return false;
> >> }
> >>
> >> if (os::stat(file2,&st2)< 0) {
> >> return false;
> >> }
> >>
> >>
> >> So far I've changed:
> >> M src/hotspot/os/posix/os_posix.cpp
> >> M src/hotspot/share/memory/filemap.cpp
> >> M src/hotspot/share/memory/filemap.hpp
> >>
> >> I'll do more testing before sending an updated webrevs.
> >>
> >> thanks,
> >> Calvin
> >>> Best regards,
> >>>
> >>> Jiangli
More information about the hotspot-runtime-dev
mailing list