[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]

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.


- 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.


- 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) {

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.

- 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;

Best regards,

Jiangli