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

[Lindenmaier, Goetz]

Hi Coleen, 

yes, we have a Coverity team inhouse, and I keep 
giving them input to improve the tool. Also, I marked
far more items as "false positive" in the database than
I actually fix.  

The change passed jdk/submit, (jdk/submit11 was not 
reachable) and all our testing, so I think I can push 
this now.

Best regards,
  Goetz.

> -----Original Message-----
> From: coleen.phillimore at oracle.com [mailto:coleen.phillimore at oracle.com]
> Sent: Donnerstag, 12. Juli 2018 14:04
> 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.
> 
> 
> 
> On 7/12/18 5:02 AM, Lindenmaier, Goetz wrote:
> > Hi Coleen,
> >
> > thanks for looking at my change.
> >
> > New webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/02/
> >
> >> + name ? name->as_C_string() : "<unnamed>");
> >> Can you change to:
> >> + name != NULL ? name->as_C_string() : "<unnamed>");
> > Fixed.
> >
> >> 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 removed it and leave it to Lois.
> 
> Great. that's even better.
> >
> >> 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 the case with the null name is for "MISSING_NAME"
> > and thus no message is needed here. (If range is null no
> > message is printed either.)
> 
> Okay this is fine.   For the record, if you think these are false
> positives, you should file a report against the static analysis tool but
> I think you should still make these simple changes.
> 
> Thanks,
> Coleen
> >
> > Best regards,
> >    Goetz.
> >
> >
> >> 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.