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

[Lindenmaier, Goetz]

Hi David, 

Take your time (within the RDP1 timeframe ��) to look at the issues themselves. 
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

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

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