RFR(XS): 8169734: Update uses of string "java.base" to macro
Rachel Protacio
rachel.protacio at oracle.com
Mon Dec 5 17:24:49 UTC 2016
Thank you!!
Rachel
On 12/5/2016 12:16 PM, 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.
>
> 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