[aarch64-port-dev ] RFR (XS) 8229123: Revert build fixes for aarch64/zero

[Andrew Dinn]

On 05/08/2019 09:54, Aleksey Shipilev wrote:
> RFE:
>   https://bugs.openjdk.java.net/browse/JDK-8229123
> 
> Another thing found during 8u-aarch64 webrev scrubbing. Not sure why this thing is needed. Reverting
> it does not break aarch64/zero cross-compiled build. There is no change like this in any of the
> upstreams either. Maybe Edward remembers why it is there?

I ecrtainy don't recognise this code. Maybe Andrew Haley does?

Is Ed Nevill's name on the change-set? If so he will probably have had a
good reason to add this. Given the comment explaining why this is needed
I'm concerned that the error it guards against may only turn up in
unusual circumstances which might well explain why standard tests
succeeded after removal. Still ...

> 8u-aarch64 fix:
> 
> diff -r 732ebff024df src/share/vm/interpreter/interpreterRuntime.cpp
> --- a/src/share/vm/interpreter/interpreterRuntime.cpp   Mon Aug 05 09:42:26 2019 +0200
> +++ b/src/share/vm/interpreter/interpreterRuntime.cpp   Mon Aug 05 10:51:59 2019 +0200
> @@ -396,37 +396,35 @@
>    int                handler_bci;
>    int                current_bci = bci(thread);
> 
> -#ifndef CC_INTERP
>    if (thread->frames_to_pop_failed_realloc() > 0) {
>      // Allocation of scalar replaced object used in this frame
>      // failed. Unconditionally pop the frame.
>      thread->dec_frames_to_pop_failed_realloc();
>      thread->set_vm_result(h_exception());
>      // If the method is synchronized we already unlocked the monitor
>      // during deoptimization so the interpreter needs to skip it when
>      // the frame is popped.
>      thread->set_do_not_unlock_if_synchronized(true);
>  #ifdef CC_INTERP
>      return (address) -1;
>  #else
>      return Interpreter::remove_activation_entry();
>  #endif
>    }
> -#endif
> 
>    // Need to do this check first since when _do_not_unlock_if_synchronized
>    // is set, we don't want to trigger any classloading which may make calls
> 
> Testing: aarch64 {server,zero} builds; ad-hoc testing zero build
... Yuck! Those ifdefs are a mess which does not inspire confidence.
Since this is not present and has not yet caused a problem in jdk11u or
head I can see why it is very tempting to delete it and wait for the
error to turn up. However, that's just what we *ought not* to do in a
maintenance release. So, I don't think that's a good enough reason to
remove it.

I think the only way to proceed here would be to 1) look into how the
value returned by thread->frames_to_pop_failed_realloc() can be > 0 and
see if the logic of this response really makes sense 2) check with Ed
Nevill why he added the code (assuming it was his) 3) work out if ths
code really is needed to protect against a potential error and 4) still
leave it alone unless those 3 prior investigations provide an utterly
sound reason to delete it. Alternatively, you can claim priority of the
status quo as force majeure and do nothing. Your choice :-)

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander