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

[Lois Foltan]

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.

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