RFR (M) CR 8050147: StoreLoad barrier interferes with stack usages

[John Rose]

On Aug 19, 2014, at 3:30 AM, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:

> Thanks, Vladimir.
> 
> Coming back to this...
> 
> On 08/12/2014 02:30 AM, Vladimir Kozlov wrote:
>> You may need to modify Compile::bang_size_in_bytes() and
>> LIR_Assembler::bang_size_in_bytes() to bang correct number of pages. Try
>> to test with small stacks and see
>> test/compiler/uncommontrap/StackOverflowGuardPagesOff.java.
>> 
>> Related problems fixed recently:
>> https://bugs.openjdk.java.net/browse/JDK-8026775
>> https://bugs.openjdk.java.net/browse/JDK-8032410
> 
> Ugh, what a mess. I think I understand the essence for those two issues.
> The test you mention there passes on current and patched builds, but
> that tells me nothing.
> 
> To fix this properly, I need some guidance. If we are shooting for "lock
> addl (%esp, -OFFSET)" for a StoreLoad barrier, does it mean we should
> bang for (frame_size + OFFSET) bytes? If so, is this the addition we are
> shooting for?
> 
> diff -r 184aac46be1f src/share/vm/c1/c1_LIRAssembler.cpp
> --- a/src/share/vm/c1/c1_LIRAssembler.cpp	Sun Aug 10 19:38:53 2014 -0700
> +++ b/src/share/vm/c1/c1_LIRAssembler.cpp	Tue Aug 19 14:18:12 2014 +0400
> @@ -169,7 +169,7 @@
> // removes the need to bang the stack in the deoptimization blob which
> // in turn simplifies stack overflow handling.
> int LIR_Assembler::bang_size_in_bytes() const {
> -  return MAX2(initial_frame_size_in_bytes(),
> _compilation->interpreter_frame_size());
> +  return MAX2(initial_frame_size_in_bytes() +
> VM_Version::L1_line_size(), _compilation->interpreter_frame_size());
> }
> 
> void LIR_Assembler::emit_exception_entries(ExceptionInfoList* info_list) {
> diff -r 184aac46be1f src/share/vm/opto/compile.cpp
> --- a/src/share/vm/opto/compile.cpp	Sun Aug 10 19:38:53 2014 -0700
> +++ b/src/share/vm/opto/compile.cpp	Tue Aug 19 14:18:12 2014 +0400
> @@ -430,7 +430,7 @@
> // removes the need to bang the stack in the deoptimization blob which
> // in turn simplifies stack overflow handling.
> int Compile::bang_size_in_bytes() const {
> -  return MAX2(_interpreter_frame_size, frame_size_in_bytes());
> +  return MAX2(_interpreter_frame_size, frame_size_in_bytes() +
> VM_Version::L1_line_size());
> }
> 
> This makes me uneasy for two reasons:
> 
> a) We only need to do this for x86. ifdef'ing the generic
> LIR_Assembler/Compile code seems odd. Can we pro-actively bang for an
> additional cache line for all platforms?

I do think we could do this, although there are ways it could bite us later.

It would need a comment saying that it matters for x86, and is thought to be harmless for other chips.  That way if someone wants to change it later on they will know the constraints.

But, to be more verbose but also more future-proof, let's add another function to these files:
  hotspot/src/os_cpu/*/vm/os_*.hpp

    /* amount beyond the callee frame size that we bang the stack; see JDK-@@ */
    static int extra_bang_size_in_bytes() { return VM_Version::L1_line_size(); }

> b) The cache line size here does seem like a magic number, and I would
> like to move the StoreLoad SP offset calculation somewhere -- but where
> to? VM_Version seems to be too generic to handle the x86-specific case.
> Putting it in assembler(_x86) seems to introduce the artificial
> dependency between the machine-neutral and machine-dependent parts of
> the compiler.

The function above could easily couple to x86-specific logic.

— John