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

[Liu Xin]

Hi, Martin,

On Fri, Sep 6, 2019 at 9:36 AM Doerr, Martin <martin.doerr at sap.com> wrote:

> Hi Xin,
>
>
>
> thanks for looking into this.
>
>
>
> I think it goes too far for this issue. If you would like to introduce
> macro_overloading.hpp, this should be done in a separate issue. It will
> need to get reviewed on hotspot-runtime-dev, not on hotspot-compiler-dev.
>

Got it. thanks. I vaguely remember there're more than one place have the
similar demand.
variadic macros can simplify macro expression.  I will file an issue about
it if I find more places in hotspot.


>
>
> Also note that your version of c1_LIRAssembler_s390.cpp misses the latest
> changes from
>
> http://cr.openjdk.java.net/~mdoerr/8230669_s390_label_asserts/webrev.02/
>
I am good with your CR.

>
>
> c1_Compilation.hpp contains a typo: bailed_out_out()
>
>
>
> > Can I assume that all our target compilers support c99?
>
> Supported build platforms with compiler information is documented here:
>
> https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms
>
> I believe that all compilers do support C99.
>
This is an additional reason why this should be done in a separate issue:
>
> The fix for this issue will probably need to get backported, but your new
> code may cause trouble when backporting.
>
>
>
> Best regards,
>
> Martin
>
>
>
>
>
> *From:* Liu Xin <navy.xliu at gmail.com>
> *Sent:* Freitag, 6. September 2019 17:51
> *To:* Doerr, Martin <martin.doerr at sap.com>
> *Cc:* Hohensee, Paul <hohensee at amazon.com>; Liu, Xin <xxinliu at amazon.com>;
> 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,
>
>
>
> How about this webrev?
>
> https://cr.openjdk.java.net/~xliu/8230669/00/webrev/
>
>
>
> macro_overloading.hpp makes use of VARIADIC MACRO, which is in C99
> Standard.
>
> https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html
>
>
>
> Can I assume that all our target compilers support c99?
>
>
>
> thanks,
>
> --lx
>
>
>
>
>
>
>
> On Fri, Sep 6, 2019 at 8:27 AM Doerr, Martin <martin.doerr at sap.com> wrote:
>
> 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
>
>