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

Mon Aug 20 17:22:14 UTC 2018

Hi, Gunter, 
It looks good to me, but I am not a reviewer. 

Now I can explain why LIR_Assembler::bailout doesn't work out. 
LinearScan has its own bailout. BAILOUT/BAILOUT_ in c1_LinearScan.cpp will use LinearScan::bailout. It won't reset LIRAssembler:: _unwind_handler_entry.

Actually,  not only LinearScan, c1_GraphBulider , FpuStackAllocator all has private bailout methods. I ignored this fact at first place.  Your reset in dtor is correct. 

Thanks,
--lx

On 8/20/18, 7:23 AM, "Haug, Gunter" <gunter.haug at sap.com> wrote:

    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