RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Ioi Lam
ioi.lam at oracle.com
Thu Mar 14 06:46:00 UTC 2019
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
> <mailto: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
>> <mailto: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
>>> <mailto: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 <mailto: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
>>>>> <mailto: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
>>>>> <mailto: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
>>>>>> <mailto: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
>>>>>> <mailto: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>
>>>>>> >>>> <mailto: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