RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Thu Mar 14 16:54:57 UTC 2019
I was going to propose eliminating the usage of is_modules_image(const
char*) as well. I'm in the middle of refactoring ClassFileStream to take
the 'source' as a ClassPathEntry* instead of a plain char*. I think that's
a more elegant approach. However, this issue needs to be addressed in JDK
11 and 12 also. So a simpler fix should be applied first and backported to
11 & 12. The webrev.02 or webrev.03 can be used for the initial fix and
backporting. Then we can be unblocked by this symlink issue.
The elimination of is_modules_image(const char*) and refactoring work can
be done with a different RFE. If you want to take over from there, that is
ok with me.
Thanks,
Jiangli
On Wed, Mar 13, 2019 at 11:46 PM Ioi Lam <ioi.lam at oracle.com> wrote:
> I have a different idea ....
>
> I think we are all confused by is_modules_image(const char* name). It's a
> poorly designed interface -- it's not clear what it's intended for, and
> what its capabilities are (Can it handle case-insensitivity? Can it handle
> non-canonical names? Should it do a magic number check to see if the file
> is indeed a jimage file?).
>
> I think we should remove this function, otherwise it might be abused in
> the future and cause more problems.
>
> We should replace it with what we actually need. I.e., check if a
> ClassFileStream was created from the jimage.
>
> I have written a simple fix and it passes all CDS tests, tier1/tier2, as
> well as Jiangli's new test
>
>
> http://cr.openjdk.java.net/~iklam/jdk13/8220095_remove_is_modules_image.v00/
>
> Any thoughts?
>
> Thanks
> - Ioi
>
>
>
> On 3/13/19 6:24 PM, Jiangli Zhou wrote:
>
>
>
> On Wed, Mar 13, 2019 at 1:54 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
>>
>> On 3/13/2019 1:45 PM, Jiangli Zhou wrote:
>>
>> On Wed, Mar 13, 2019 at 12:21 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>>
>>> On 3/13/2019 12:03 PM, Jiangli Zhou wrote:
>>>
>>>
>>>
>>> On Wed, Mar 13, 2019 at 11:53 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>>>
>>>> On 3/13/2019 11:41 AM, Jiangli Zhou wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Mar 13, 2019 at 11:03 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>
>>>>> Hi Jiangli,
>>>>>
>>>>> Thanks for making the changes. This makes the code much simpler and
>>>>> fixes the original problem with the improper use of MODULES_IMAGE_NAME.
>>>>>
>>>>> [1] Naming and comments:
>>>>>
>>>>> modules_image_identity: how about modules_image_canonical_path, so we
>>>>> know what it actually is?
>>>>>
>>>>
>>>> The modules_image_identity naming appears better as the canonical path
>>>> is used as an identity here. It tells the intention of the variable.
>>>>
>>>>
>>>>>
>>>>> is_modules_image(const char* name) --> is_modules_image(const char*
>>>>> canonical_path)
>>>>>
>>>>
>>>> I considered to change the name of the argument but decided against
>>>> it. The sources of the argument can come from a ClassPathEntry name in some
>>>> of the places. It creates confusion without also change the other existing
>>>> code. A separate exercise can be used to clean up the existing code.
>>>>
>>>>
>>>>>
>>>>> class ClassPathImageEntry ... {
>>>>> + // returns canonical path of <JAVA_HOME>/lib/<MODULES_IMAGE_NAME>
>>>>> const char* name() const { return _name == NULL ? "" : _name; }
>>>>>
>>>>>
>>>>> [2] 142 const char* ClassLoader::_modules_image_identity =
>>>>> MODULES_IMAGE_NAME;
>>>>>
>>>>> ... but your version of is_modules_image() assumes the uninitialized
>>>>> value is NULL.
>>>>>
>>>>
>>>> Changed to initialize it to NULL.
>>>>
>>>>
>>>>>
>>>>> [3] For the following code:
>>>>>
>>>>> 452 static bool is_modules_image(const char* name) {
>>>>> 453 if (Arguments::has_jimage()) {
>>>>> 454 assert(modules_image_identity() != NULL, "must be set");
>>>>> 455 return strcmp(name, modules_image_identity()) == 0;
>>>>> 456 }
>>>>> 457 return false;
>>>>> 458 }
>>>>>
>>>>> Instead of maintaining a copy of the jimage's name in
>>>>> _modules_image_identity, how about
>>>>>
>>>>> const char* modules_image_identity() {
>>>>> assert(get_jrt_entry() != NULL, "must be inited");
>>>>> return get_jrt_entry()->name();
>>>>> }
>>>>>
>>>>> Also, I think in most cases, the name is exactly the same pointer as
>>>>> get_jrt_entry()->name(), so line 455 can be changed to
>>>>>
>>>>> const char* id = modules_image_identity();
>>>>> return name == id || strcmp(name, id) == 0;
>>>>>
>>>>> Also, I think you should change
>>>>> ClassPathImageEntry::is_modules_image() to simply return true. Otherwise
>>>>> it's just comparing its name with its name .... If you do this, then you
>>>>> can add another assert to my version of modules_image_identity(), without
>>>>> worrying above infinite recursion:
>>>>>
>>>>> assert(get_jrt_entry()->is_modules_image(), "must be");
>>>>>
>>>>
>>>> I'm hesitate to use ClassPathImageEntry name directly because it may
>>>> not necessary to be full canonical path. Using a separate variable for
>>>> modules identity seems to be more explicit and less error-prone in the
>>>> future.
>>>>
>>>> But your code essentially just makes a copy of
>>>> get_jrt_entry()->name()? How does that address your concern?
>>>>
>>>
>>> True, it makes an assumption about the existing implementation, which
>>> might not be future-proof. One solution is to change
>>> create_class_path_entry() to give back the canonical path explicitly.
>>>
>>> Our problem is not strictly limited to canonical paths. All of the
>>> callers that expect a positive return from is_modules_image() get their
>>> input from get_jrt_entry()->name(). For example, see
>>> ClassLoader::load_class(). The caller wants to know "is the src, supplied
>>> in a ClassFileStream, the same as get_jrt_entry()".
>>>
>>> -> Just grep with 'is_modules_image[(][^)]'. There are very few callers.
>>>
>>> The problem is get_jrt_entry()->name() may be subject to transformation,
>>> so that it's not necessarily JAVA_HOME/lib/modules. In this particular bug,
>>> the transformation is pathname canonicalization. It's possible to use other
>>> transformations. E.g., change it to "?modules?".
>>>
>>> So the check we really want to do is: strcmp(name,
>>> get_jrt_entry()->name());
>>>
>>
>> There are mixed sources of the path strings when calling into
>> is_modules_image(const char*). The ClassFileStream::_source is one and
>> ClassPathEntry->name() is another case. The ClassFileStream::_source is the
>> canonical path of the class file stream.
>>
>>
>> ClassLoader::load_class() {
>> ...
>> stream = _jrt_entry->open_stream(file_name, CHECK_NULL);
>> ...
>>
>> ClassFileStream* ClassPathImageEntry::open_stream(const char* name,
>> TRAPS) {
>> ...
>> return new ClassFileStream((u1*)data,
>> (int)size,
>> _name,
>> ClassFileStream::verify);
>>
>>
>> So the ClassFileStream contains the same string as in the _jrt_entry?
>>
>> Where do you see this "_source is the canonical path of the class file
>> stream" happening?
>>
>
> You are right, the ClassFileStream created for the runtime image uses
> ClassPathImageEntry::_name as the _source. So there is only a single source
> of the path string, which is jrt_entry '_name'. Using that within the check
> probably is ok then.
>
> Thanks,
> Jiangli
>
>>
>>
>> If the ClassPathEntry->name() changes as you describe above (which is
>> also the source of my hesitation), then using get_jrt_entry()->name() in
>> the check would cause an issue. That's also the reason why I didn't think a
>> simple strcmp was a good idea.
>>
>> If you are pushing to use get_jrt_entry()->name() and do a simple strcmp,
>> then it needs to guarantee get_jrt_entry()->name() will always give the
>> full canonical path. Not sure if a comment with a warning
>> in ClassPathImageEntry class is sufficient?
>>
>> Thanks,
>> Jiangli
>>
>>> Thanks
>>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 3/13/19 10:22 AM, Jiangli Zhou wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Mar 12, 2019 at 4:42 PM Jiangli Zhou <jianglizhou at google.com>
>>>>> wrote:
>>>>>
>>>>>> One of my early experimental revision actually simply used strcmp to
>>>>>> compare the full path and to also recorded the full path for the canonical
>>>>>> modules name.
>>>>>>
>>>>>> I decide to not go with that because there is no guarantee that the
>>>>>> path given to is_modules_image check is in canonical form and it diverts
>>>>>> from the existing assumptions about the usages of this API (checking the
>>>>>> file name only). The "/dev/fd/0" example you have is not a direct result of
>>>>>> the current fix but is an inherent issue of the modules check. Any file
>>>>>> with the same designated modules name would pass the check. If we want to
>>>>>> safely use the full path comparison, we need to closely exam all the call
>>>>>> sites and make sure all the places use canonical paths. I'll some analysis
>>>>>> on that.
>>>>>>
>>>>>
>>>>> I've double checked all existing call sites of is_modules_image(const
>>>>> char*). No extra processing is done (e.g. extracting the file name) on the
>>>>> path strings when passing to is_modules_image. Using the full string
>>>>> comparison appears to be safe so I have no objection. I've reverted my
>>>>> change to record the complete canonical path as the modules identity and
>>>>> changed to do full path string comparison:
>>>>>
>>>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.02/
>>>>>
>>>>> Local testing looks good. I've also submitted a submit-repo testing.
>>>>> Please help rerun tier2, tier3 and maybe some of the higher tier tests due
>>>>> to the semantics change of is_modules_image.
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>> On Tue, Mar 12, 2019 at 4:13 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>>
>>>>>>> This seems more complicated that it needs to be.
>>>>>>>
>>>>>>> + The VM can have at most one modules file.
>>>>>>>
>>>>>>> + The only reason the VM calls is_modules_image(p) is to check
>>>>>>> whether p, a full path, is THE modules file. In all cases, p is actually
>>>>>>> derived from the singleton ClassPathImageEntry::name(). It's just that in
>>>>>>> some cases, such as in the logging code, we have lost the pointer to the
>>>>>>> ClassPathImageEntry, and just have the full path.
>>>>>>>
>>>>>>> + On line 722 of
>>>>>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.01/src/hotspot/share/classfile/classLoader.cpp.sdiff.html,
>>>>>>> you already have a full path of THE modules file.
>>>>>>>
>>>>>>> Why can't you just do a strcmp of the full path? That's much
>>>>>>> simpler, and more importantly, correct. Your version will return true for
>>>>>>> "/dev/fd/0" for the test case described in the bug report. That doesn't
>>>>>>> seem to be correct.
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 3/12/19 9:10 AM, Jiangli Zhou wrote:
>>>>>>>
>>>>>>> Hi Ioi,
>>>>>>>
>>>>>>> Thanks for looking into this also.
>>>>>>>
>>>>>>> On Mon, Mar 11, 2019 at 8:19 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/11/19 7:30 PM, Ioi Lam wrote:
>>>>>>>> > I think this patch should be further improved.
>>>>>>>> >
>>>>>>>> > First of all, the original code is problematic:
>>>>>>>> >
>>>>>>>> > static bool is_modules_image(const char* name) { return
>>>>>>>> > string_ends_with(name, MODULES_IMAGE_NAME); }
>>>>>>>> >
>>>>>>>> > So if we have a file called /foo/bar/xxxmodules, this function
>>>>>>>> returns
>>>>>>>> > true!
>>>>>>>> >
>>>>>>>>
>>>>>>>> BTW, If you have a symlink JAVA_HOME/lib/modules ->
>>>>>>>> JAVA_HOME/lib/r,
>>>>>>>> then your new version of is_modules_image will return true for any
>>>>>>>> jar
>>>>>>>> file.
>>>>>>>>
>>>>>>>> See
>>>>>>>>
>>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/f984aca565c1/src/hotspot/share/oops/instanceKlass.cpp#l3370
>>>>>>>>
>>>>>>>> if (ClassLoader::is_modules_image(cfs->source())) {
>>>>>>>> info_stream.print(" source: jrt:/%s", module_name);
>>>>>>>> }
>>>>>>>>
>>>>>>>> This will cause all classes loaded from modular JAR files be
>>>>>>>> incorrectly
>>>>>>>> printed as "jrt://"
>>>>>>>>
>>>>>>>> Even if you also use the last "/" path separator, it's still not
>>>>>>>> safe to
>>>>>>>> use the filename alone to determine whether a file is indeed the
>>>>>>>> modules
>>>>>>>> image file. I.e., what if you have JAVA_HOME/lib/modules ->
>>>>>>>> JAVA_HOME/lib/test.jar?
>>>>>>>>
>>>>>>>
>>>>>>> The current bug addresses the practical issue when the modules is a
>>>>>>> sym link to a unique content identifier. Using modules as a link to any
>>>>>>> arbitrary file is not addressed by this bug. It's in the same area but a
>>>>>>> different issue.
>>>>>>>
>>>>>>> If lib/modules ->lib/test.jar, or lib/modules -> ../ (or any other
>>>>>>> arbitrary file/dir) is done, the VM fails differently. This is the same as
>>>>>>> if you replace libjvm.so with another random file.
>>>>>>>
>>>>>>> The canonical modules name is obtained from the actual modules image
>>>>>>> during setup_boot_search_path, so it's guaranteed to refer to the correct
>>>>>>> identifier. Checking the file name after '/' is safe.
>>>>>>>
>>>>>>> The substring issue that you pointed out has not been observed, as
>>>>>>> that's prevented by the naming method of the content identifiers. The
>>>>>>> substring issue however can be solved relatively inexpensively by making
>>>>>>> sure the check covers the entire file name after '/'.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Also, I think we can have a version that's simpler than what I
>>>>>>>> proposed
>>>>>>>> below:
>>>>>>>>
>>>>>>>> + We can have at most a single ClassPathImageEntry.
>>>>>>>> + All calls to ClassLoader::is_modules_image() are made after
>>>>>>>> the
>>>>>>>> ClassPathImageEntry is created
>>>>>>>>
>>>>>>>> so:
>>>>>>>>
>>>>>>>> static bool is_modules_image(const char* name) {
>>>>>>>> if (ClassPathImageEntry::singleton() != NULL) {
>>>>>>>> return strcmp(name,
>>>>>>>> ClassPathImageEntry::singleton()->name()) == 0);
>>>>>>>> } else {
>>>>>>>> assert(_exploded_entries != NULL, "must be running with
>>>>>>>> exploded
>>>>>>>> modules");
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>> The argument passed can be a path to the modules, so strcmp cannot
>>>>>>> be simply used for 'name'.
>>>>>>>
>>>>>>> static bool is_modules_image(const char* name) {
>>>>>>>
>>>>>>> if (Arguments::has_jimage()) {
>>>>>>>
>>>>>>> assert(_modules_image_name != NULL, "must be set");
>>>>>>>
>>>>>>> const char* p = skip_directories(name);
>>>>>>>
>>>>>>> return strcmp(p, _modules_image_name) == 0;
>>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> return false;
>>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> I've revised the is_modules_image() to also cover the substring
>>>>>>> issue.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> This function should be changed to always return true
>>>>>>>>
>>>>>>>> bool ClassPathImageEntry::is_modules_image() const {
>>>>>>>> - return ClassLoader::is_modules_image(name());
>>>>>>>> + return true;
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>> Leaving ClassPathImageEntry::is_modules_image() without changing is
>>>>>>> safer, so it can make sure the ClassPathImageEntry name agrees with the
>>>>>>> canonical modules name.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> To make sure we don't call is_modules_image before
>>>>>>>> ClassPathImageEntry
>>>>>>>> is created, we could do
>>>>>>>>
>>>>>>>> static bool is_modules_image(const char* name) {
>>>>>>>> DEBUG_ONLY(_is_modules_image_called = true;)
>>>>>>>> ...
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>> The 'assert(_modules_image_name != NULL, "must be set")' check makes
>>>>>>> sure is_modules_image(const char*) is not called
>>>>>>> before setup_boot_search_path. There is no need to add additional flag.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ClassPathImageEntry::ClassPathImageEntry() {
>>>>>>>> assert(_singleton == NULL, "...");
>>>>>>>> assert(!_is_modules_image_called, "Don't call
>>>>>>>> ClassLoader::is_modules_image() before ClassPathImageEntry is
>>>>>>>> allocated");
>>>>>>>> ...
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> - Ioi
>>>>>>>>
>>>>>>>>
>>>>>>>> > It seems the patch simply makes the code more convoluted.
>>>>>>>> Instead,
>>>>>>>> > shouldn't we just open the file and check the magic number?
>>>>>>>> >
>>>>>>>> > If performance is important, we can use a cache, because we allow
>>>>>>>> a
>>>>>>>> > single module image:
>>>>>>>> >
>>>>>>>> > static bool is_modules_image(const char* name) {
>>>>>>>> > static const char* singleton_module_name = NULL;
>>>>>>>> >
>>>>>>>> > if (singleton_module_name != NULL) {
>>>>>>>> > return strcmp(name, singleton_module_name) == 0;
>>>>>>>> > }
>>>>>>>> > // open <name> and check magic number
>>>>>>>> > if (good) {
>>>>>>>> > singleton_module_name = os::strdup(name);
>>>>>>>> > return true;
>>>>>>>> > }
>>>>>>>> > return false;
>>>>>>>> > }
>>>>>>>> >
>>>>>>>> > Thanks
>>>>>>>> > - Ioi
>>>>>>>> >
>>>>>>>> > On 3/11/19 10:51 AM, Jiangli Zhou wrote:
>>>>>>>> >> On Mon, Mar 11, 2019 at 8:44 AM Calvin Cheung <
>>>>>>>> calvin.cheung at oracle.com>
>>>>>>>> >> wrote:
>>>>>>>> >>
>>>>>>>> >>> Hi Jiangli,
>>>>>>>> >>>
>>>>>>>> >>> The updated webrev looks good.
>>>>>>>> >>>
>>>>>>>> >>> On 3/10/19, 6:41 PM, Jiangli Zhou wrote:
>>>>>>>> >>>> Here is the updated webrev:
>>>>>>>> >>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.01/
>>>>>>>> >>>> <http://cr.openjdk.java.net/%7Ejiangli/8220095/webrev.01/>.
>>>>>>>> >>>>
>>>>>>>> >>>> - Incorporated all suggestions except the test platforms for
>>>>>>>> >>>> ModulesSymLink.java. I changed to requires linux, mac and
>>>>>>>> solaris
>>>>>>>> >>>> explicitly instead to avoid any potential issue with other
>>>>>>>> >>>> non-mainline testing platforms.
>>>>>>>> >>>> - Reverted the changes in sharedPathsMiscInfo.cpp. Further
>>>>>>>> testing
>>>>>>>> >>>> showed SharedPathsMiscInfo::check() were dealing the system
>>>>>>>> boot path
>>>>>>>> >>>> string set up by os::set_boot_path. Lois comments reminded me
>>>>>>>> to
>>>>>>>> >>>> double check with this. Thanks!
>>>>>>>> >>>>
>>>>>>>> >>>> Please help fire tier2 and tier3 runs with mach5.
>>>>>>>> >>> I ran tier2 and tier3 tests with your latest changes and saw no
>>>>>>>> test
>>>>>>>> >>> failures.
>>>>>>>> >>>
>>>>>>>> >> Great, thanks a lot!
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >>> BTW, I was unable to download the jdk.patch - got "403 -
>>>>>>>> forbidden"
>>>>>>>> >>> error.
>>>>>>>> >>> Same error was seen with individual patch files.
>>>>>>>> >>> I used wget to get all the raw files into my repo and then
>>>>>>>> submitted
>>>>>>>> >>> the
>>>>>>>> >>> build and test job.
>>>>>>>> >>> Could you check the file permissions?
>>>>>>>> >>>
>>>>>>>> >> Yes, that's the file permission issue. I fixed the permission
>>>>>>>> for the
>>>>>>>> >> jdk.patch.
>>>>>>>> >>
>>>>>>>> >> Thanks!
>>>>>>>> >> Jiangli
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >>> thanks,
>>>>>>>> >>> Calvin
>>>>>>>> >>>> Thanks!
>>>>>>>> >>>> Jiangli
>>>>>>>> >>>>
>>>>>>>> >>>> On Thu, Mar 7, 2019 at 5:11 AM Lois Foltan <
>>>>>>>> lois.foltan at oracle.com
>>>>>>>> >>>> <mailto:lois.foltan at oracle.com>> wrote:
>>>>>>>> >>>>
>>>>>>>> >>>> On 3/6/2019 6:20 PM, Jiangli Zhou wrote:
>>>>>>>> >>>> > Please review the following fix for 8220095.
>>>>>>>> >>>> >
>>>>>>>> >>>> > webrev:
>>>>>>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.00/
>>>>>>>> >>>> <http://cr.openjdk.java.net/%7Ejiangli/8220095/webrev.00/>
>>>>>>>> >>>> > bug: https://bugs.openjdk.java.net/browse/JDK-8220095
>>>>>>>> >>>> >
>>>>>>>> >>>> > Symbolic links may be used commonly in some cloud
>>>>>>>> environments.
>>>>>>>> >>>> The target
>>>>>>>> >>>> > files might have different names than the linked names.
>>>>>>>> The VM
>>>>>>>> >>>> crashes when
>>>>>>>> >>>> > 'lib/modules' links to a target file with a different
>>>>>>>> name, for
>>>>>>>> >>>> example,
>>>>>>>> >>>> > lib/modules -> lib/0.
>>>>>>>> >>>> >
>>>>>>>> >>>> > The usage of the hard-coded MODULES_IMAGE_NAME (used by
>>>>>>>> >>>> > ClassLoader::is_modules_image(const char*)) can become
>>>>>>>> >>>> problematic in this
>>>>>>>> >>>> > case and leads to assertion failures and crashes in
>>>>>>>> this
>>>>>>>> >>>> case. The
>>>>>>>> >>>> > is_modules_image()
>>>>>>>> >>>> > checks always fail even the actual file is the runtime
>>>>>>>> image
>>>>>>>> >>>> because the
>>>>>>>> >>>> > canonical paths (which might not end with 'modules')
>>>>>>>> are passed
>>>>>>>> >>>> in for the
>>>>>>>> >>>> > check. The fix is to obtain the real name from the
>>>>>>>> canonical
>>>>>>>> >>>> path early
>>>>>>>> >>>> > during initialization and use that for the
>>>>>>>> is_modules_image()
>>>>>>>> >>> check.
>>>>>>>> >>>> >
>>>>>>>> >>>> > Tested with submit repo testing, tier1, tier2, and
>>>>>>>> hotspot
>>>>>>>> >>>> runtime tests
>>>>>>>> >>>> > locally.
>>>>>>>> >>>> >
>>>>>>>> >>>> > Thanks!
>>>>>>>> >>>> > Jiangli
>>>>>>>> >>>>
>>>>>>>> >>>> Hi Jiangli,
>>>>>>>> >>>>
>>>>>>>> >>>> This change looks good! A couple of minor comments:
>>>>>>>> >>>>
>>>>>>>> >>>> - classLoader.hpp
>>>>>>>> >>>> line #39 - I think you should expand on the comment
>>>>>>>> and add
>>>>>>>> >>>> why in
>>>>>>>> >>>> the case of the canonical path name MODULES_IMAGE_NAME
>>>>>>>> can not be
>>>>>>>> >>>> used
>>>>>>>> >>>> as-is but if one is dealing with the system boot path
>>>>>>>> string set
>>>>>>>> >>>> up by
>>>>>>>> >>>> os::set_boot_path, than MODULES_IMAGE_NAME should be used.
>>>>>>>> >>>>
>>>>>>>> >>>> - classLoader.cpp
>>>>>>>> >>>> line #702 - Can you add the same explanation comment
>>>>>>>> as in
>>>>>>>> >>>> classLoader.hpp ahead of the new
>>>>>>>> >>>> ClassLoader::init_modules_image_name()
>>>>>>>> >>>> method? That would be helpful.
>>>>>>>> >>>> line #710 - ahead of setting _modules_image_name
>>>>>>>> please
>>>>>>>> >>>> consider
>>>>>>>> >>>> adding an assert that p1's length is greater than 0?
>>>>>>>> >>>> Maybe there should there be an
>>>>>>>> error case
>>>>>>>> >>>> test
>>>>>>>> >>>> where "lib/modules -> lib/"
>>>>>>>> >>>>
>>>>>>>> >>>> Thanks,
>>>>>>>> >>>> Lois
>>>>>>>> >>>>
>>>>>>>> >>>> -
>>>>>>>> >>>>
>>>>>>>> >
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>
More information about the hotspot-runtime-dev
mailing list