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

[David Holmes]

On 10/07/2018 10:32 PM, Lindenmaier, Goetz wrote:
> Hi David,
> 
> Take your time (within the RDP1 timeframe ��) to look at the issues themselves.

I have 24 hours left before I'm on vacation and I won't be around to 
argue about this. Other's seem quite happy to do whatever the checks say 
regardless of whether it is really necessary or not. So be it.

> Just for the basic comments on this:
>> I see some asserts changed to guarantees which is unnecessary in general
>> - but again appeases static checkers looking at product builds.
> This has been done to a large extend for parfait:
>   http://hg.openjdk.java.net/jdk/jdk/search/?rev=parfait&revcount=40

In many cases we file "false positive" bugs against parfait or just 
ignore these warnings. We typically use these kind of asserts to sanity 
check preconditions that are statically known. So when we run the code 
in debug mode and it doesn't assert we know it won't encounter bad 
values in product mode either.

I don't agree with changing them just to pacify a static analysis tool. 
The fact the code path may not be performance sensitive is not in itself 
justification to me. "Performance death by a thousand cuts"

>> I also don't see this as a P3 bug, as there seems only 1 potential real
>> bug there (you yourself call these "minor improvements"). So this seems
>> unsuitable for JDK 11 now we are in RDP1. But fine for 12.
> When else should I do this? I can only do this when development
> is closed, else I have to re-run and do fixes again and again for
> incoming changes.
> We are required to run the checker and fix issues before releasing
> a VM.

And you also have to comply with the RDP processes of OpenJDK. I don't 
make the rules.

And until GA you still may have to re-run to cover all the changes 
coming in - that's the nature of the beast.

If it were me releasing my own builds I'd be applying these changes 
locally and then after the release I'd upstream the patch.

But I'm not commenting further.

Cheers,
David

> Best regards,
>    Goetz.
> 
> 
> 
> 
> 
> 
> 
> http://hg.openjdk.java.net/jdk/jdk/search/?rev=parfait&revcount=40
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Dienstag, 10. Juli 2018 14:10
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(M): 8206977: Minor improvements of runtime code.
>>
>> Hi Goetz,
>>
>> On 10/07/2018 8:53 PM, 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/
>>
>> It will take a while to go through these. I see some false positives
>> caused by too local an examination - which is typical of code checkers.
>> For example in os_linux.cpp
>>
>> +     if (buflen > 7) {
>>
>> we know the buffer coming in is O_BUFLEN in size. Add an assert if you
>> like but no need for a hard-wired guard.
>>
>> I see some asserts changed to guarantees which is unnecessary in general
>> - but again appeases static checkers looking at product builds.
>>
>> I also don't see this as a P3 bug, as there seems only 1 potential real
>> bug there (you yourself call these "minor improvements"). So this seems
>> unsuitable for JDK 11 now we are in RDP1. But fine for 12.
>>
>> Will try to go through in more detail tomorrow, but it is somewhat
>> tedious to have to work through these in detail to refute the code
>> checkers claims of incorrectness.
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> 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.
>>>