RFR(M): 8207049: Minor improvements of compiler code.

[Lindenmaier, Goetz]

Hi Vladimir, 

thanks for reviewing my changes. I saw your comment 
in the bug that it passed your tests. I also pushed it 
to jdk/submit (submit11 was not reachable) and our tests
which both passed without errors.

Best regards,
  Goetz.

> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Mittwoch, 11. Juli 2018 21:43
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot compiler
> <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: RFR(M): 8207049: Minor improvements of compiler code.
> 
> Nice changes, Goetz
> 
> I agree with changes. Let me test them.
> 
> Thanks,
> Vladimir
> 
> On 7/11/18 7:23 AM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > I ran coverity on the jdk11 hotspot sources and want to propose the
> following fixes. 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.  If you consider some too conservative,
> I'm happy to remove them.
> >
> > http://cr.openjdk.java.net/~goetz/wr18/8207049-covCompiler/01/
> >
> > Real issues
> > -----------
> >
> > x86.ad
> >    For Op_CMoveVF the code always returned 'false'.
> >
> > compileLog.cpp
> >    read returns -1 on unsuccesful completion. Use a
> >    singned variable and check for error before
> >    subtracting -1 from to_read etc.
> >
> > disassembler.cpp
> >    check for _library.
> >
> > jvmciCompilerToVM.cpp, getResolvedJavaType
> >    base_object is checked for null above, so
> >    check before printing the name, too.
> >
> > simpleThresholdPolicy.cpp
> >    If I understand correctly, if the compile queue is empty,
> >    no max_task will be found. This is quite unlikely, though.
> >
> >
> >
> > Useful code improvements:
> > -------------------------
> >
> > codeBuffer.cpp
> >    Above, next_cs is checked for NULL. I would assume this
> >    check is boguous, but checking below, too, is on the
> >    safe side.
> >
> >    The codebuffer should be aligned properly, so the
> >    first section does not need alignment.
> >    Make this more clear in the code. Else it looks
> >    as if prev_dest_cs is dereferenced when NULL.
> >
> > c1_IR.cpp
> >    It's not obvious that n->subst() != n if false
> >    on the first iteration.
> >
> > c1_LinearScan.cpp
> >    spill_candidate is used as index to an array in
> >    _mapping_from.at() and thus should not be negative.
> >
> >    prev_cmp is dereferenced further down. change to guarantee.
> >
> > codeCache.cpp
> >    Use format specifyer to print strings.
> >
> > methodLiveness.cpp
> >    Turn assert into guarantee().
> >
> > oopMap.cpp
> >    Change loop so that last is always initilaized.
> >    last gets dereferenced below.
> >
> >
> > jvmciCodeInstaller.cpp
> >    DebugInformationRecorder::describe_scope might pass NULL.
> >
> > arraycopy.cpp
> >    If c == NULL the condition of the next if will crash.
> >    Change to guarantee.
> >
> > bytecodeInfo.cpp
> >    If callee_method ever had been NULL, we would have
> >    seen a crach or assertion in post_inlining_event.
> >
> > callnode.cpp
> >    Would fail if IR is broken.
> >
> > compile.cpp
> >    Don't access arrays at negative indexes.
> >
> > gcm.cpp
> >    There must be an LCA, as there should be at least one
> >    ancestor.
> >
> > ifnode.cpp
> >    It's unclear that a proj is found in if (vop == Op_Phi)
> >
> > indexSet.cpp
> >    Wrong data type used.
> >
> > lcm.cpp
> >    Turn assertion into guarantee, don't access worklist at -1.
> >
> > loopPredicate.cpp
> >    limit is dereferenced by _igvn.type(limit)
> >
> > loopTransform.cpp
> >    if limit_n == NULL, we will skip the following if and do _igvn.type(limit_n)
> >    either return false, a guarantee or assert would do, too, I think.
> >
> > macro.cpp
> >    igvn.replace_node(fallthroughproj, ctrl) would fail if ctrl == NULL.
> >
> > memnode.cpp
> >    igvn is checked for NULL above, too. Protect all uses similarly.
> >
> >    In the default case load == NULL would be dereferenced below.
> >
> > node.hpp
> >    arr is checked for NULL above. Maybe remove (arr == NULL) check
> above?
> >
> > output.cpp
> >    turn assertion into guarantee
> >
> > parse1.cpp
> >    turn assertion into guarantee
> >
> > reg_split.cpp
> >    turn assertion into guarantee
> >
> > runtime.cpp
> >    print "no method" if m == NULL (assigned NULL above).
> >
> > type.cpp
> >    One of the if blocks should be reached in any case.
> >    Change else if into a guarantee.
> >
> >
> > It's obviously pointless to scan adlc, but fixing the
> > issues does not hurt either I think.
> >
> > archDesc.cpp
> >    Above, node is checked for NULL.
> >
> > arena.cpp
> >    memset writes bytes.
> >
> > dfa.cpp, valid_loc():
> >    breaks are missing.
> > dfa.cpp, compute_external():
> >    Use strncat (should also use snprintf, but we still have
> >    VS 2013 which does not support that)
> >
> > filebuff.cpp
> >    breaks are missing.
> >
> > formssel.cpp
> >    name_left was just checked for NULL.
> >    form2 is checked for NULL above, so check needed here, too.
> >
> > main.cpp
> >    Wrong delete operator used.
> >
> > output_c.cpp
> >    We cannot continue after this error, as below
> >    inst_rep_var is dereferenced.
> >    opc is assigned NULL if local == NULL. (in both cases).
> >