RFR: 8185723: Zero: segfaults on Power PC 32-bit

[Severin Gehwolf]

Hi,

On Wed, 2017-08-02 at 17:35 +0100, Andrew Haley wrote:
> We've been seeing baffling segfaults in the C++ interpreter on Power
> PC 32-bit.
> 
> Zero is so called because it uses zero assembly language, but this is
> not quite true: there is a tiny bit of assembly language, and it is
> wrong.  Here is the PPC32 definition of atomic_copy64.  It uses a
> floating-point register to copy a 64-bit doubleword atomically:
> 
>   // Atomically copy 64 bits of data
>   static void atomic_copy64(volatile void *src, volatile void *dst) {
> #if defined(PPC32) && !defined(__NO_FPRS__)
>     double tmp;
>     asm volatile ("lfd  %0, 0(%1)\n"
>                   "stfd %0, 0(%2)\n"
>                   : "=f"(tmp)
>                   : "b"(src), "b"(dst));
> 
> The eagle-eyed among you might have noticed the bug: this asm has no
> memory effect.  It has no memory inputs, no memory outputs, and no
> memory clobber.  So, as far as GCC is concerned atomic_copy64 does not
> touch memory at all, and there is no need to store the source operand
> into memory.  For all GCC knows, the asm might just be doing some
> arithmetic on the pointers.  We need a better definition of
> atomic_copy64, and this is mine:
> 
>   // Atomically copy 64 bits of data
> static void atomic_copy64(volatile void *src, volatile void *dst) {
> #if defined(PPC32) && !defined(__NO_FPRS__)
>     double tmp;
>     asm volatile ("lfd  %0, %2\n"
>                   "stfd %0, %1\n"
>                   : "=&f"(tmp), "=Q"(*(volatile double*)dst)
>                   : "Q"(*(volatile double*)src));
> 
> Note that we dereference src and dst and pass the actual memory
> operands to the asm, not just pointers to them.
> 
> http://cr.openjdk.java.net/~aph/8185723/
> 
> OK?

FWIW, with this patch OpenJDK 9 Zero bootcyle-images builds for me on
PPC32 in the release variant.

That used to segv prior this patch. Slowdebug builds were OK pre-patch. 
Issue was only observable with fastdebug/release builds.

If it matters, we also know that passing -fno-tree-dce to the hotspot
build "fixed" the issue pre-patch.

+1 from me.

Thanks,
Severin