RFR (XS): 8208480: Fix for test failure: assert(is_bound() || is_unused()) after JDK-8206075 in C1

[Haug, Gunter]

Hi all,

Here is the updated webrev:

http://cr.openjdk.java.net/~ghaug/webrevs/8208480.v1/

I have removed the redundant reset of the label.

Best regards,
Gunter

On 20.08.18, 10:00, "Haug, Gunter" <gunter.haug at sap.com> wrote:

    Hi all,
    
    the bail-out path originates from  Compilation::emit_code_epilog not from LIR_Assembler. The design using the CHECK_BAILOUT() macro is a bit unfortunate in this case. I found it the easiest solution to reset the label in the destructor. 
    
    In fact you're right, my change makes the original bail-out support redundant, I'll prepare an updated webrev.
    
    Best regards,
    Gunter
    
    
    
    From: Liu Xin <navy.xliu at gmail.com>
    Date: Friday, 17. August 2018 at 21:03
    To: Lutz Schmidt <lutz.schmidt at sap.com>
    Cc: "Haug, Gunter" <gunter.haug at sap.com>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>, "Doerr, Martin" <martin.doerr at sap.com>, "xxinliu at amazon.com" <xxinliu at amazon.com>
    Subject: Re: RFR (XS): 8208480: Fix for test failure: assert(is_bound() || is_unused()) after JDK-8206075 in C1
    
    hi, Lutz and Gunter, 
    
    Thank you for providing the information. 
    I met the full codecache in C1 in compiler/codegen/TestCharVect2.java
    In my understanding,  Compilation::emit_code_epilog(LIR_Assembler* assembler) checks bailout for every statement. 
    Do you meet the problem only on s390? 
    
    I am not object to your patch. if you need to reset  the label in c1_LIRAssember's dtor, I think bailout  change  is redundant. 
    Remove it make code clearer. 
    
    diff -r fbb62267e5e9 src/hotspot/share/c1/c1_LIRAssembler.cpp
    --- a/src/hotspot/share/c1/c1_LIRAssembler.cpp
    Thu Aug 09 15:52:23 2018 -0700
    +++ b/src/hotspot/share/c1/c1_LIRAssembler.cpp
    Fri Aug 17 11:57:21 2018 -0700
    @@ -112,6 +112,9 @@
    
    
     LIR_Assembler::~LIR_Assembler() {
    +  // The unwind handler label may be unbound if this destructor is invoked because of a bail-out.
    +  // Reset it here to avoid an assertion.
    +  _unwind_handler_entry.reset();
     }
    
    
    diff -r fbb62267e5e9 src/hotspot/share/c1/c1_LIRAssembler.hpp
    --- a/src/hotspot/share/c1/c1_LIRAssembler.hpp
    Thu Aug 09 15:52:23 2018 -0700
    +++ b/src/hotspot/share/c1/c1_LIRAssembler.hpp
    Fri Aug 17 11:57:21 2018 -0700
    @@ -71,11 +71,7 @@
       void record_non_safepoint_debug_info();
    
       // unified bailout support
    -  void bailout(const char* msg) {
    -    // reset the label in case it hits assertion in destructor.
    -    _unwind_handler_entry.reset();
    -    compilation()->bailout(msg);
    -  }
    +  void bailout(const char* msg) const { compilation()->bailout(msg); }
       bool bailed_out() const                        { return compilation()->bailed_out(); }
    
       // code emission patterns and accessors
    
    
    On Fri, Aug 17, 2018 at 8:35 AM Schmidt, Lutz <mailto:lutz.schmidt at sap.com> wrote:
    Hi, 
     
    I’m responding on Gunter’s behalf. He just left the office a few minutes ago.
     
    The bailout case was a “codecache full” condition. For the error to occur, you must run into that condition when a reference to the label is already generated, but the label is not bound yet. This is not JCK test specific. We just happened to hit “codecache full” there. 
     
    “jck_simple_api” is a subset of the api suite we compiled for our own purposes. The tests contained therein are just “simple” tests: they do not run too long, they can run massively parallel, they do not depend on specific resources (like ports, addresses, …).
     
    I’m afraid this doesn’t help you much. At least I tried. �� If you can’t reproduce on your own, please let us know what tracing you need. We could then try to produce the output on our test systems. 
     
    Thanks,
    Lutz
     
    From: Liu Xin <mailto:navy.xliu at gmail.com>
    Date: Friday, 17. August 2018 at 17:15
    To: "Haug, Gunter" <mailto:gunter.haug at sap.com>
    Cc: "mailto:hotspot-runtime-dev at openjdk.java.net" <mailto:hotspot-runtime-dev at openjdk.java.net>, Lutz Schmidt <mailto:lutz.schmidt at sap.com>, "Doerr, Martin (mailto:martin.doerr at sap.com)" <mailto:martin.doerr at sap.com>, "mailto:xxinliu at amazon.com" <mailto:xxinliu at amazon.com>
    Subject: Re: RFR (XS): 8208480: Fix for test failure: assert(is_bound() || is_unused()) after JDK-8206075 in C1
     
    hi, Gunter, 
    I do consider bailout case. why didn't it catch your case? 
    c1_LIRAssember.hpp
     // unified bailout support
      void bailout(const char* msg) {
        // reset the label in case it hits assertion in destructor.
        _unwind_handler_entry.reset();
        compilation()->bailout(msg);
      }
     
    can I reproduce this jck_simple_api_work?
    /priv/jvmtests/output_sapjvm12_o_jdk-test_dbgU_linuxs390x/jck_simple_api_work/hs_err_pid108809.log
     
    On Fri, Aug 17, 2018 at 6:39 AM Haug, Gunter <mailto:gunter.haug at sap.com> wrote:
    Hi all,
    
    could I please have  a review and a sponsor for this tiny fix:
    
    https://bugs.openjdk.java.net/browse/JDK-8208480
    http://cr.openjdk.java.net/~ghaug/webrevs/8208480
    
    C1 holds a label to the unwind handler during compilation. There are bail-out paths where a branch to this label has already been emitted but the handler hasn't (e.g. code cache full). The label is therefore unbound when the destructor is invoked and the assertion fires.
    
    Thanks,
    Gunter