RFR(M): 8206977: Minor improvements of runtime code.

[coleen.phillimore at oracle.com]

On 7/10/18 3:10 PM, Lois Foltan wrote:
> Hi Goetz,
>
> Just a couple of comments based on Coleen's review, see below.
>
> On 7/10/2018 9:03 AM, coleen.phillimore at oracle.com wrote:
>
>> http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/01/src/hotspot/share/classfile/moduleEntry.cpp.udiff.html 
>>
>>
>> + name ? name->as_C_string() : "<unnamed>");
>
> Instead of "<unnamed>" please use the UNNAMED_MODULE macro from 
> moduleEntry.hpp.
>
>>
>>
>> Can you change to:
>>
>> + name != NULL ? name->as_C_string() : "<unnamed>");
>>
>>
>> http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/01/src/hotspot/share/oops/klassVtable.cpp.udiff.html 
>>
>>
>> This looks a lot nicer!   Similar code is in linkResolver.cpp, can 
>> you look at changing it too?
>
> I have an RFR out currently for JDK-8205611, (see 
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-June/033325.html), 
> which needs one more reviewer's okay.  It contains changes to reword 
> the error messages for loader constraint violations in order to follow 
> the new proposed format for module and class loader information.  So 
> our two changes will conflict in this area.

Goetz, if you want to check in this to 11, I think Lois can change it 
similarly in the linkResolver code with her change for 12, once it 
migrates down.

Thanks,
Coleen

>
> Thanks,
> Lois
>
>>
>> http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/01/src/hotspot/share/services/writeableFlags.cpp.udiff.html 
>>
>>
>> If name is null here, what would this do?  Should there be an 'else' 
>> to print something?
>>
>> I think this looks fine.  It doesn't look major to me.  The asserts 
>> turned to guarantees don't appear to be anywhere performance sensitive.
>>
>> Thanks,
>> Coleen
>>
>>
>>
>> On 7/10/18 6:53 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> I ran coverity on the jdk11 hotspot sources and want to propose the
>>> following fixes to the runtime code. I scanned the linux x86_64 build.
>>> Some issues are similar to previous parfait fixes (check for NULL, add
>>> guarantees etc.) I also identified some issues I consider real 
>>> problems.
>>> http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/01/
>>>
>>> In detail:
>>>
>>> Real issues:
>>> ------------
>>>
>>> jvmtiEnvBase.cpp
>>>    || should be &&.
>>>    Attention, this is the only change that really will change 
>>> behaviour.
>>>    But if thr == NULL we will see a crash below.
>>>
>>> perfMemory_linux.cpp:
>>>    Wrong buffer length used.
>>>
>>> systemDictionary.cpp:
>>>    Move code dereferencing ik under if (ik != NULL).
>>>
>>> virtualspace.cpp
>>>    Initialization is missing. Moved constructor up to the other
>>>    constructors.
>>>
>>>
>>> Useful code improvements:
>>> -------------------------
>>>
>>> vm_version_ext_x86.cpp
>>>    Assure buffer is not accessed at offset -1.
>>>
>>> os_linux.cpp
>>>    Numa_max_node returns int, and a -1 in some cases.
>>>
>>> moduleEntry.cpp
>>>    name might be NULL. Just a fix for tracing.
>>>
>>> systemDictionaryShared.cpp
>>>    clearify code.
>>>    It would be wrong if only entry == NULL would hold, one
>>>    would hit the assertion below.
>>>
>>> verifier.cpp
>>>    Fix tracing.
>>>    Illegal opcode is -1 and should not be passed to name array.
>>>
>>> logOutput.cpp
>>>    If n_selections == 0, best_selection would be NULL.
>>>    Move up the assertion and turn into a guarantee.
>>>
>>> filemap.cpp
>>>    Either base can be NULL, or parts of the code before are dead.
>>>
>>> metaspace.cpp
>>>    We now an exception is pending.
>>>
>>> klassVtable.cpp
>>>    Coverity does not like the format in a variable.
>>>    Anyways this is quite rough coding, transformed to use stringStream
>>>    as with other similar exceptions.
>>>
>>> jvmFlag.cpp
>>>    match might be NULL.
>>>
>>> writableFlags.cpp
>>>    name might be NULL.
>>>
>>> ostream.cpp
>>>    If ftell returns error code -1, we need not continue.
>>>    Especially we should not fseek(-1).
>>>
>>> logTestUtils.inline.hpp
>>>    ftell returns -1.
>>>
>>> test_metachunk.cpp
>>>    wrong datatype.
>>
>