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

[Lois Foltan]

On 7/22/2016 12:46 PM, Calvin Cheung wrote:
> Hi Lois,
>
> Just some minor comments:
>
> classLoader.cpp:
> 821   jio_snprintf(path, len, "%s%cmodules%c%s", home, file_sep, 
> file_sep, module_name);
>
> I think it is better to add a #define for "modules" since it is also 
> being used in os.cpp:
> 1217   char* base_classes = format_boot_path("%/modules/java.base", 
> home, home_len, fileSep, pathSep);
> though I'm not sure if the #define will work with format_boot_path().
Hi Calvin,
I did look at this, but at this point would prefer a subsequent fix.

>
> It's fine with me if you choose to fix it in a subsequent fix.
>
> nit: line too long in the following:
>
> filemap.cpp
>
> 205     assert(jrt_entry != NULL, "No modular java runtime image 
> present when allocating the CDS classpath entry table");
Done.

>
> classLoaderExt.hpp
>
> 73   static void add_class_path_entry(const char* path, bool 
> check_for_duplicates, ClassPathEntry* new_entry) {
Done.

Thanks for the review!
Lois

>
> thanks,
> Calvin
>
> On 7/21/16, 2:11 PM, Lois Foltan 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
>>>>
>>>
>>