RFR (L) JDK-8154239: -Xbootclasspath/a breaks exploded build

[Jiangli Zhou]

Hi Lois,

> On Jul 25, 2016, at 11:38 AM, Lois Foltan <lois.foltan at oracle.com> wrote:
> 
> 
> On 7/22/2016 6:45 PM, Jiangli Zhou wrote:
>> Hi Lois,
>> 
>> This looks really good. I have one very minor comment for ClassLoader::add_to_list(). How about adding an assert to make sure _first_append_entry is also NULL under ‘if (_last_append_entry == NULL)’ before setting the entries?
> Done.
> 
> Thanks for the review Jiangli!  For completeness, here is a webrev that incorporates all review comments:
> 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8154239.2/webrev/

Looks good. Thank you for adding the exploded module build checks in FileMapHeader::validate() and Arguments::check_unsupported_dumping_properties().

Thanks,
Jiangli

> 
> Lois
> 
>> 
>> Thanks,
>> Jiangli
>> 
>> 
>>> On Jul 21, 2016, at 2:11 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>> 
>>> 
>>> On 7/21/2016 11:25 AM, harold seigel wrote:
>>>> Hi Lois,
>>>> 
>>>> This is a nice fix for the exploded build system class path issues!
>>>> 
>>>> If the callers of methods such as add_to_exploded_build_list() already have THREAD, can it be passed as a parameter?
>>> Done.
>>> 
>>>> In classLoader.cpp, the comment at lines 1466 - 1468 say that the starting classpath_index is  always 1.  Can you either add an assert for this or change line 1469 to just: classpath_index = 1;
>>> Done.
>>> 
>>>> In classLoader.hpp, could has_jimage()  be renamed to something like has_jrt_entry so it's not confused with the has_jimage() method in arguments.hpp?
>>> Done.
>>> 
>>>> In filemap.cpp, can the code at lines 226 - 230 be changed to:
>>>> 
>>>>   assert(strptr + name_bytes <= strptr_max, "miscalculated buffer size");
>>>>   strncpy(...)
>>>>   strptr += name_bytes;
>>> Done.
>>> 
>>> Thanks for the review Harold!  New webrev at http://cr.openjdk.java.net/~lfoltan/bug_jdk8154239.1/webrev/
>>> Lois
>>> 
>>>> Thanks, Harold
>>>> 
>>>> 
>>>> On 7/19/2016 2:09 PM, Lois Foltan wrote:
>>>>> Hello,
>>>>> 
>>>>> Please review the following fix:
>>>>> 
>>>>> Webrev:
>>>>>    http://cr.openjdk.java.net/~lfoltan/bug_jdk8154239/webrev/
>>>>> 
>>>>> Bug: -Xbootclasspath/a breaks exploded build
>>>>>    https://bugs.openjdk.java.net/browse/JDK-8154239
>>>>> 
>>>>> Summary:
>>>>> The JVM was incorrectly handling how to set up and search the ClassPathEntry's for directories representing the module definitions in an exploded module build situation for the boot class loader. The incorrect set up and search was particularly exposed when -Xbootclasspath/a was specified in conjunction with a exploded module build.  In an exploded module build, when Modules::define_modules was called to define a module to the boot loader, a ClassPathEntry was simply appended to the boot loader's search list.  At load class time, a class would then be searched by simply traversing that list without any regards to the module that class was defined in.  This is incorrect behavior.  The class should only be searched for in the location of the module's definition that it was defined within.  The fix now records each module and it's exploded build location in order to adhere to this rule.
>>>>> 
>>>>> The other piece to this problem was that if -Xbootclasspath/a was specified, it was incorrectly injected prior to the majority of ClassPathEntry's for the exploded modules, yielding a broken and incorrect search order for the boot loader.  Introducing the concept of a "base" or "core" piece for the boot loader which is either the java runtime image or the exploded modules build and changing the ClassLoader::_first_append_entry to truly be only appended entries added via -Xbootclasspath/a or jvmti appended entries, introduces a clean way for how this information is stored in the ClassLoader data structure that maps directly to how ClassLoader::load_class() searches that information.
>>>>> 
>>>>> 215   // The boot class path consists of 3 ordered pieces:
>>>>> 216   //  1. the module/path pairs specified to -Xpatch
>>>>> 217   // -Xpatch:<module>=<file>(<pathsep><file>)*
>>>>> 218   //  2. the base piece
>>>>> 219   //    [jimage | build with exploded modules]
>>>>> 220   //  3. boot loader append path
>>>>> 221   //    [-Xbootclasspath/a]; [jvmti appended entries]
>>>>> 222   //
>>>>> 223   // The boot loader must obey this order when attempting
>>>>> 224   // to load a class.
>>>>> 
>>>>> 
>>>>> Testing:
>>>>> Full hotspot nightly testing with a java runtime image build
>>>>> JDK java/io, java/lang, java/util with both a java runtime image and an exploded module build
>>>>> JCK lang & vm with both a java runtime image and an exploded module build
>>>>> Hotspot jtreg tests with both a java runtime image and an exploded module build
>>>>> 
> 
>