RFR(XS): 8169734: Update uses of string "java.base" to macro
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Dec 6 19:24:43 UTC 2016
+1
On 12/6/16 11:22, Daniel D. Daugherty wrote:
> A low priority JDK10 bug is fine by me. Just want it to be clean...
> Thanks!
>
> Dan
>
>
> On 12/6/16 12:14 PM, Rachel Protacio wrote:
>> Hi! Thanks for your comments; unfortunately, I had committed before I
>> got these replies... If you'd like me to address this in a bug,
>> please let me know.
>> Rachel
>>
>> On 12/5/2016 4:55 PM, serguei.spitsyn at oracle.com wrote:
>>> On 12/5/16 13:50, Daniel D. Daugherty wrote:
>>>> A reply to Serguei's last comment below...
>>>>
>>>> On 12/5/16 2:05 PM, Rachel Protacio wrote:
>>>>> 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);
>>>>
>>>> Doesn't seem that weird to me:
>>>
>>> I admit that I used wrong term ("weird") here. :)
>>> It looks a little bit unusual to me.
>>>
>>>>
>>>> char* base_classes = format_boot_path("%/modules/java.base", home,
>>>> home_len, fileSep, pathSep);
>>>>
>>>> changes to:
>>>>
>>>> char* base_classes = format_boot_path("%/modules/" JAVA_BASE_NAME,
>>>> home, home_len, fileSep, pathSep);
>>>>
>>>> which is normal C++ string concatenation...
>>>
>>>
>>> I have no objection to fix it this way.
>>> Thank you for commenting it.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>>>>
>>>>>> 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