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

[Lindenmaier, Goetz]

Hi Martin,

thank's for your review!

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Donnerstag, 12. Juli 2018 16:21
> 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.
> 
> Hi Götz,
> 
> I have looked at all your changes. There are quite some good findings. And
> the other ones look at least ok to me. I couldn't see any mistakes. Reviewed.
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
> Sent: Mittwoch, 11. Juli 2018 16:23
> To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
> Subject: RFR(M): 8207049: Minor improvements of compiler code.
> 
> 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 tooo
> 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).