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

[Lindenmaier, Goetz]

Hi David, 

> Sorry Goetz this comes over far too abrupt, which wasn't my intention.
No, it's fine. I know you are always a critical reviewer, but also always
constructive and very thoroughly.  I wish you a fine vacation!

Best regards,
  Goetz.



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