RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused()) failed: Label was never bound to a location, but it was used as a jmp target

[Doerr, Martin]

Hi Götz, Paul and Xin,

thank you for reviewing and for your input. Pushed.
@Götz: I forgot to add you as reviewer. Sorry for that.

Best regards,
Martin


> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Montag, 9. September 2019 14:35
> To: Doerr, Martin <martin.doerr at sap.com>; Hohensee, Paul
> <hohensee at amazon.com>; Liu, Xin <xxinliu at amazon.com>; 'hotspot-
> compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>
> Subject: RE: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> failed: Label was never bound to a location, but it was used as a jmp target
> 
> Hi Martin,
> 
> the fix looks good to me.
> 
> Thanks,
>   Goetz.
> 
> > -----Original Message-----
> > From: hotspot-compiler-dev <hotspot-compiler-dev-
> > bounces at openjdk.java.net> On Behalf Of Doerr, Martin
> > Sent: Freitag, 6. September 2019 17:27
> > To: Hohensee, Paul <hohensee at amazon.com>; Liu, Xin
> > <xxinliu at amazon.com>; 'hotspot-compiler-dev at openjdk.java.net'
> <hotspot-
> > compiler-dev at openjdk.java.net>
> > Subject: [CAUTION] RE: RFR(XS): 8230669: [s390] C1: assert(is_bound() ||
> > is_unused()) failed: Label was never bound to a location, but it was used as
> a
> > jmp target
> >
> > Hi Paul,
> >
> > sure, I can move it there if you would like to reuse it for other platforms:
> >
> http://cr.openjdk.java.net/~mdoerr/8230669_s390_label_asserts/webrev.0
> 2/
> >
> > c1_CodeStubs_s390.cpp doesn’t use any scope local Labels before the
> > CHECK_BAILOUT.
> > So I think I’ve already fixed all obvious places. Let me know if you find more
> > ones.
> > Best regards,
> > Martin
> >
> >
> > From: Hohensee, Paul <hohensee at amazon.com>
> > Sent: Freitag, 6. September 2019 16:20
> > To: Doerr, Martin <martin.doerr at sap.com>; Liu, Xin
> <xxinliu at amazon.com>;
> > 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net>
> > Subject: Re: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> > failed: Label was never bound to a location, but it was used as a jmp target
> >
> >
> > I looked for definitions of CHECK_BAILOUT and found two. One is in
> > c1_Compilation.hpp.
> >
> >
> > #define CHECK_BAILOUT()            { if (bailed_out()) return;          }
> > #define CHECK_BAILOUT_(res)        { if (bailed_out()) return res;      }
> >
> >
> >
> > The other is in c1_CodeStubs_s390.cpp.
> >
> >
> > #define CHECK_BAILOUT() { if (ce->compilation()->bailed_out()) return; }
> >
> >
> >
> > The code in c1_LIRAssembler_s390.cpp uses the definition from
> > c1_Compilation.hpp. That being the case, I’d add the CHECK_BAILOUT
> macros
> > to c1_Compilatioh.hpp, because it’s where the original CHECK_BAILOUT
> macro
> > is defined, and we may find cases outside s390 where they’re needed. I’d
> also
> > take a look at c1_CodeStubs_s390.cpp for possible problems.
> >
> >
> >
> > Thanks,
> >
> >
> >
> > Paul
> >
> > From: "Doerr, Martin"
> > <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
> > Date: Friday, September 6, 2019 at 2:16 AM
> > To: "Hohensee, Paul"
> > <hohensee at amazon.com<mailto:hohensee at amazon.com>>, "Liu, Xin"
> > <xxinliu at amazon.com<mailto:xxinliu at amazon.com>>, "'hotspot-
> compiler-
> > dev at openjdk.java.net'" <hotspot-compiler-
> > dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>>
> > Subject: RE: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> > failed: Label was never bound to a location, but it was used as a jmp target
> >
> > Hi Xin and Paul,
> >
> > thanks for your quick response and your helpful comments.
> >
> > I have added the defines to c1_LIRAssembler_s390.cpp because other
> > platforms don’t use the CHECK_BAILOUT macro in their platform
> assembler.
> >
> > Indeed, there are more places at which we need to reset labels.
> >
> > Please review my new version:
> >
> http://cr.openjdk.java.net/~mdoerr/8230669_s390_label_asserts/webrev.0
> 1/
> >
> > Best regards,
> > Martin
> >
> >
> > From: Hohensee, Paul
> > <hohensee at amazon.com<mailto:hohensee at amazon.com>>
> > Sent: Donnerstag, 5. September 2019 21:29
> > To: Liu, Xin <xxinliu at amazon.com<mailto:xxinliu at amazon.com>>; Doerr,
> > Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>;
> 'hotspot-
> > compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>>
> > Subject: Re: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> > failed: Label was never bound to a location, but it was used as a jmp target
> >
> > You don’t need overloaded macros, you can just use, e.g.,
> >
> >
> > #define CHECK_BAILOUT()                        { if (bailed_out()) return; }
> >
> > #define CHECK_BAILOUT1(label1)                 { if (bailed_out()) label1.reset();
> > return; }
> >
> > #define CHECK_BAILOUT2(label1, label2)         { if (bailedout())
> label1.reset();
> > label2.reset(); return; }
> >
> > #define CHECK_BAILOUT2(label1, label2, label3) { if (bailedout())
> label1.reset();
> > label2.reset(); label3.reset() return; }
> >
> >
> > Paul
> >
> > From: "Liu, Xin" <xxinliu at amazon.com<mailto:xxinliu at amazon.com>>
> > Date: Thursday, September 5, 2019 at 12:22 PM
> > To: "Doerr, Martin"
> <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>,
> > "Hohensee, Paul"
> > <hohensee at amazon.com<mailto:hohensee at amazon.com>>, "'hotspot-
> > compiler-dev at openjdk.java.net'" <hotspot-compiler-
> > dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>>
> > Subject: Re: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> > failed: Label was never bound to a location, but it was used as a jmp target
> >
> > Hi, Martin,
> >
> > The intention is to catch malformed code in assembler. It also helps
> developers
> > to write assembly code.
> > Indeed,  the assertion is kind of useless in bailout path. I sympathize your
> > feeling.
> >
> > your fix  is correct.  I found c1_LIRAssembler_s390.cpp has yet another
> place
> > has similar pattern – line 2574.  Could you cover it as well?
> >
> > BAILOUT cases scatter everywhere. I don’t want to see you get bitten
> again. I
> > have a idea. How about we  group all local Labels and give them a shared
> state.
> > The state indicates that the current code path is in bailout or not.
> > Does it make sense?
> >
> > Bailout state;
> > {
> >
> > Label  a(state)
> > NearLabel b(state);
> > NearLabel c(state);
> >
> > If (bailout) {
> >   state.set();
> >   // dtors of Label skip assert
> >   return ;
> > }
> >
> > // dtors uses assert
> > }
> >
> > Paul suggests to use overloaded macro for CHECK_BAILOUT.  It might bring
> > compiler compatibility problem. Let me search hotspot code to see if it has
> > supported overloaded and variadic macros.
> >
> > Thanks,
> > --lx
> >
> >
> >
> > From: "Doerr, Martin"
> > <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
> > Date: Thursday, September 5, 2019 at 9:57 AM
> > To: "Liu, Xin" <xxinliu at amazon.com<mailto:xxinliu at amazon.com>>,
> > "Hohensee, Paul"
> > <hohensee at amazon.com<mailto:hohensee at amazon.com>>, "'hotspot-
> > compiler-dev at openjdk.java.net'" <hotspot-compiler-
> > dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>>
> > Subject: RFR(XS): 8230669: [s390] C1: assert(is_bound() || is_unused())
> failed:
> > Label was never bound to a location, but it was used as a jmp target
> >
> > Hi,
> >
> > the time bomb JDK-8206075<https://bugs.openjdk.java.net/browse/JDK-
> > 8206075> has hit us again (3rd time):
> > https://bugs.openjdk.java.net/browse/JDK-8230669
> >
> > I think the recommended way to deal with it is:
> >
> http://cr.openjdk.java.net/~mdoerr/8230669_s390_label_asserts/webrev.0
> 0/
> >
> > Please review.
> >
> > Best regards,
> > Martin