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

[Lindenmaier, Goetz]

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.02/
> 
> 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.01/
> 
> 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.00/
> 
> Please review.
> 
> Best regards,
> Martin