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

[Calvin Cheung]

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

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

classLoaderExt.hpp

73   static void add_class_path_entry(const char* path, bool 
check_for_duplicates, ClassPathEntry* new_entry) {

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