RFR (XS): 8208480: Fix for test failure: assert(is_bound() || is_unused()) after JDK-8206075 in C1
Haug, Gunter
gunter.haug at sap.com
Fri Aug 24 13:27:05 UTC 2018
Change pushed, sorry for the delay!
Best regards,
Gunter
On 22.08.18, 18:43, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
No, it is not urgent. I just wondering if someone forgot to push it.
Thank you for letting me know.
Vladimir
On 8/22/18 9:34 AM, Schmidt, Lutz wrote:
> Simple reason: Gunter is not in. Maybe he is back in the office Thursday, Friday for sure.
> If it's urgent, feel free to push.
>
> Thanks,
> Lutz
>
> On 22.08.18, 18:30, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
>
> What is blocking push?
>
> Testing passed on our side.
>
> Thanks,
> Vladimir
>
> On 8/20/18 3:13 PM, Hohensee, Paul wrote:
> > LGTM (I'm a reviewer).
> >
> > Thanks,
> >
> > Paul
> >
> > On 8/20/18, 10:27 AM, "hotspot-runtime-dev on behalf of Liu, Xin" <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of xxinliu at amazon.com> wrote:
> >
> > 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
> >
> >
> >
> >
> >
> >
> >
>
>
More information about the hotspot-compiler-dev
mailing list