RFR(XS): 8169734: Update uses of string "java.base" to macro
Rachel Protacio
rachel.protacio at oracle.com
Mon Dec 5 21:05:12 UTC 2016
Great, thank you Serguei! I'll commit.
Rachel
On 12/5/2016 4:03 PM, serguei.spitsyn at oracle.com wrote:
> Hi Rachel,
>
>
> It is good, thanks!
>
>
> On 12/5/16 09:16, Daniel D. Daugherty wrote:
>> On 12/5/16 9:41 AM, Rachel Protacio wrote:
>>> Thanks, Dan! Ok, here's the webrev with all the instances changed.
>>> http://cr.openjdk.java.net/~rprotacio/8169734.01/
>>
>> src/share/vm/classfile/classLoader.cpp
>> No comments.
>>
>> src/share/vm/classfile/javaClasses.cpp
>> No comments.
>>
>> src/share/vm/classfile/moduleEntry.cpp
>> No comments.
>>
>> src/share/vm/classfile/modules.cpp
>> No comments.
>>
>> src/share/vm/classfile/packageEntry.cpp
>> No comments.
>>
>> src/share/vm/classfile/vmSymbols.hpp
>> No comments.
>>
>> src/share/vm/oops/arrayKlass.cpp
>> No comments.
>>
>> src/share/vm/oops/instanceKlass.cpp
>> No comments.
>>
>> src/share/vm/runtime/arguments.cpp
>> No comments.
>>
>> Thumbs up!
>>
>> I did not do a "completeness" check. I presume that Serguei will
>> do that when he re-reviews.
>
> It seems complete to me.
> There is one more instance of naked "java.base" though:
> runtime/os.cpp: char* base_classes =
> format_boot_path("%/modules/java.base", home, home_len, fileSep,
> pathSep);
>
> but it seems weird to use the macro in the path name.
>
>
> Thanks,
> Serguei
>
>>
>> Dan
>>
>>
>>>
>>> Rachel
>>>
>>>
>>> On 12/2/2016 4:18 PM, Daniel D. Daugherty wrote:
>>>> > I'd suggest to collect at least one more opinion/review on this.
>>>>
>>>> Let's get this correct now and not have to come back and fix it down
>>>> the road. Looks to me like these new locations can take advantage
>>>> of C++ string concatenation unless I'm missing something, e.g.:
>>>>
>>>> 775 "Incorrect boot loader search path, no java
>>>> runtime image or java.base exploded build");
>>>>
>>>> becomes:
>>>>
>>>> 775 "Incorrect boot loader search path, no java
>>>> runtime image or " JAVA_BASE " exploded build");
>>>>
>>>> which is a bit long, but not too hard to deal with... And since we're
>>>> migrating away from an informal limit of 80 chars per line... I won't
>>>> even complain... :-)
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 12/2/16 1:52 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Rachel,
>>>>>
>>>>>
>>>>> On 12/2/16 11:12, Rachel Protacio wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks for the review. Yeah, we had thought about that. Two
>>>>>> concerns: (1) the concatenation or %s is a little verbose and
>>>>>> possibly excessive for these ancillary error messages, and (2)
>>>>>> because they are error messages, potentially someone could
>>>>>> hard-code something to expect the output and if for some reason
>>>>>> we change the macro in the future it could throw people off
>>>>>> unnecessarily... Just some thoughts.
>>>>>
>>>>> Yes, I was thinking about the same.
>>>>>
>>>>>
>>>>>> I'm happy to change all of them though if that's what we want.
>>>>>
>>>>> I'd suggest to collect at least one more opinion/review on this.
>>>>> It is Ok with me to keep it as it is.
>>>>>
>>>>>
>>>>>> I'll definitely change the classfile/vmSymbols.hpp and
>>>>>> runtime/os.cpp instances.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>> Let me know what you think!
>>>>>>
>>>>>> Rachel
>>>>>>
>>>>>> On 12/1/2016 5:43 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Hi Rachel,
>>>>>>>
>>>>>>> It looks good in general.
>>>>>>> However, there are more instances of java.base that may need an
>>>>>>> update.
>>>>>>>
>>>>>>> classfile/classLoader.cpp:
>>>>>>>
>>>>>>> 775 "Incorrect boot loader search path, no java
>>>>>>> runtime image or java.base exploded build");
>>>>>>>
>>>>>>> 1826 vm_exit_during_initialization("Unable to create
>>>>>>> ModuleEntry for java.base");
>>>>>>>
>>>>>>>
>>>>>>> classfile/modules.cpp:
>>>>>>>
>>>>>>> 180 "Bad package name for module: java.base");
>>>>>>>
>>>>>>> 185 err_msg("Invalid package name: %s for
>>>>>>> module: java.base", package_name));
>>>>>>>
>>>>>>> 191 err_msg("Duplicate package name: %s for
>>>>>>> module java.base",
>>>>>>>
>>>>>>> 209 assert(ModuleEntryTable::javabase_moduleEntry() != NULL,
>>>>>>> "No ModuleEntry for java.base");
>>>>>>>
>>>>>>> 230 assert(pkg != NULL, "Unable to create a java.base
>>>>>>> package entry");
>>>>>>>
>>>>>>> 244 "Module java.base is already defined");
>>>>>>>
>>>>>>> 253 log_debug(modules)("define_javabase_module(): Definition
>>>>>>> of module: java.base,"
>>>>>>>
>>>>>>> 261 log_trace(modules)("define_javabase_module(): creation
>>>>>>> of package %s for module java.base",
>>>>>>>
>>>>>>> 716 assert(ModuleEntryTable::javabase_defined(), "Attempt to
>>>>>>> call get_module before java.base is defined");
>>>>>>>
>>>>>>> 762 "Attempt to call get_module_from_pkg before
>>>>>>> java.base is defined");
>>>>>>>
>>>>>>> 799 "Attempt to call get_named_module before java.base
>>>>>>> is defined");
>>>>>>>
>>>>>>>
>>>>>>> runtime/arguments.cpp:
>>>>>>>
>>>>>>> 3432 vm_exit_during_initialization("Cannot specify
>>>>>>> java.base more than once to --patch-module");
>>>>>>>
>>>>>>>
>>>>>>> Some other files have it too:
>>>>>>>
>>>>>>> classfile/vmSymbols.hpp: template(java_base,
>>>>>>> "java.base") \
>>>>>>> classfile/moduleEntry.cpp: fatal("Unable to finalize module
>>>>>>> definition for java.base");
>>>>>>> classfile/moduleEntry.cpp: assert(jb_module != NULL, "java.base
>>>>>>> ModuleEntry not defined");
>>>>>>> classfile/moduleEntry.cpp: fatal("Unable to patch the module
>>>>>>> field of classes loaded prior to java.base's definition, invalid
>>>>>>> java.lang.reflect.Module");
>>>>>>> classfile/packageEntry.cpp: vm_exit_during_initialization("A
>>>>>>> non-java.base package was loaded prior to module system
>>>>>>> initialization", entry->name()->as_C_string());
>>>>>>> oops/arrayKlass.cpp: "module entry not available post
>>>>>>> java.base definition");
>>>>>>> oops/instanceKlass.cpp:
>>>>>>> assert(ModuleEntryTable::javabase_moduleEntry() != NULL,
>>>>>>> "java.base module is NULL");
>>>>>>> runtime/os.cpp: char* base_classes =
>>>>>>> format_boot_path("%/modules/java.base", home, home_len, fileSep,
>>>>>>> pathSep);
>>>>>>>
>>>>>>> So, the question is if it's worth to update the remaining
>>>>>>> instances as well.
>>>>>>> Simple concatenation or %s replacement in some rare cases could
>>>>>>> be used for it.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>> On 12/1/16 13:20, Rachel Protacio wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> Please review this little change, replacing errant uses of the
>>>>>>>> string "java.base" with the macro JAVA_BASE_NAME. Passes JPRT.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8169734
>>>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8169734.00/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Rachel
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list