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

[David Holmes]

Sorry Goetz this comes over far too abrupt, which wasn't my intention.

Cheers,
David

On 12/07/2018 5:20 PM, David Holmes wrote:
> 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.
>>>>