[10] RFR(M): 8176506: C2: loop unswitching and unsafe accesses cause crash

[Volker Simonis]

Hi Roland,

first of all, I think you still have a small error in sparc.ad. I
think the "size(8)" is not right because I see the following error at
VM startup:

#  Internal Error (ad_sparc.cpp:15486), pid=11655, tid=73
#  assert(VerifyOops || MachNode::size(ra_) <= 8) failed: bad fixed size

Current CompileTask:
C2:   2241   16       4       java.lang.StringLatin1::hashCode (42 bytes)

Stack: [0xffffffff45200000,0xffffffff45300000],
sp=0xffffffff452fadd0,  free space=1003k
Native frames: (J=compiled Java code, A=aot compiled Java code,
j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x1c7ce4c]  void VMError::report_and_die(int,const
char*,const char*,void*,Thread*,unsigned char*,void*,void*,const
char*,int,unsigned long)+0x894
V  [libjvm.so+0x1c7c4fc]  void VMError::report_and_die(Thread*,const
char*,int,const char*,const char*,void*)+0x6c
V  [libjvm.so+0xfed1dc]  void report_vm_error(const char*,int,const
char*,const char*,...)+0xc4
V  [libjvm.so+0x96b118]  unsigned
ShouldNotReachHereNode::size(PhaseRegAlloc*)const+0xd8
V  [libjvm.so+0x1923ac0]  void
Compile::shorten_branches(unsigned*,int&,int&,int&)+0x6c0
V  [libjvm.so+0x19279d4]  CodeBuffer*Compile::init_buffer(unsigned*)+0x3e4
V  [libjvm.so+0x1922d08]  void Compile::Output()+0x730
V  [libjvm.so+0xf247d0]  void Compile::Code_Gen()+0x610
V  [libjvm.so+0xf1a624]
Compile::Compile(ciEnv*,C2Compiler*,ciMethod*,int,bool,bool,bool,DirectiveSet*)+0x1894
...

Removing the "size()" instruction altogether, fixes the problem (but
there's probably a better solution).

I've also looked at your change and I think it works fine on
ppc64/s390x because we've always implemented the HaltNode by issuing a
trap instructions. So with your change (and your modified test) we
will just crash in the generates C2 method which I think is fine:

#  SIGTRAP (0x5) at pc=0x00003fff47b12bd8, pid=19883, tid=19964

J 215 c2 TestMaybeNullUnsafeAccess.test2(Ljava/lang/Object;)I (34
bytes) @ 0x00003fff47b12bd8 [0x00003fff47b12b80+0x0000000000000058]
j  TestMaybeNullUnsafeAccess.main([Ljava/lang/String;)V+49
v  ~StubRoutines::call_stub

I'm not sure if your change to the implementation of the HaltNode
(i.e. calling ShouldNotReachHere()) is really the best solution. With
that change, a crash due to an Unsafe NULL access looks as follows on
SPARC:

#  Internal Error
(/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/cpu/sparc/vm/macroAssembler_sparc.cpp:1378),
pid=29415, tid=87
#  Error: ShouldNotReachHere()

V  [libjvm.so+0x1c7cd0c]  void VMError::report_and_die(int,const
char*,const char*,void*,Thread*,unsigned char*,void*,void*,const
char*,int,unsigned long)+0x894
V  [libjvm.so+0x1c7c3bc]  void VMError::report_and_die(Thread*,const
char*,int,const char*,const char*,void*)+0x6c
V  [libjvm.so+0xfed034]  void report_vm_error(const char*,int,const
char*,const char*,...)+0xc4
V  [libjvm.so+0xfecf4c]  void report_vm_error(const char*,int,const char*)+0x64
V  [libjvm.so+0xfed3a0]  void report_should_not_reach_here(const char*,int)+0x48
V  [libjvm.so+0x1754c64]  void halt()+0x34
J 216 c2 TestMaybeNullUnsafeAccess.test2(Ljava/lang/Object;)I (34
bytes) @ 0xffffffff53412884 [0xffffffff53412820+0x0000000000000064]
j  TestMaybeNullUnsafeAccess.main([Ljava/lang/String;)V+49

I think this may be quite confusing to users/support because you can
not easily see that it is related to an illegal Unsafe access. It
looks more like an error in macroAssembler_sparc.cpp:1378 (and
ShouldNotReachHere() is usually used for errors in the VM and not to
signal user errors).

I obviously prefer the first crash, so maybe you can at least only
change the platforms which called "breakpoint()" until now?

Not sure if this is easily possible, but I think an ideal solution
would be to really execute the actual, offending load instead of
inserting a HaltNode. That would give you a nice hs_err file on all
platforms where the registers would even contain the offending address
(NULL) and the offset. I think this could simplify troubleshooting in
the case of a crash. What do you think?

Please find some other minor comments/suggestions below:

src/share/vm/opto/graphKit.cpp

+// add a check for null for which one branch can't be taken. It uses
+// and Opaque4 node that will cause the check to be removed after loop

- should probably read "// an Opaque4 node.." in the second line

src/share/vm/opto/opaquenode.hpp

+// know implicitly is always true of false but the compiler has no way
+// to prove. If during optimizations, that check becomes true of

- s/of/or/

test/compiler/unsafe/TestMaybeNullUnsafeAccess.java

- wouldn't it be safer to run the test with -Xbatch and
-XX:-UseOnStackReplacement for any case and to make it evident that
the test relays on the fact that test1() and test2() should be
compiled but not inlined ?

- you should also update the copyright on most files you've touched.

Regards,
Volker

On Wed, May 3, 2017 at 9:58 AM, Roland Westrelin <rwestrel at redhat.com> wrote:
>
>> Looks good to me. I will submit RBT testing, lets see how it goes.
>
> Thanks, Vladimir.
>
> Note that usual testing shouldn't cause the Halt to be executed so
> wouldn't test its new implementation.
>
> Roland.