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

Fri Sep 6 16:36:31 UTC 2019

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.

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/

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<mailto: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<mailto:hohensee at amazon.com>>
Sent: Freitag, 6. September 2019 16:20
To: Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>; Liu, Xin <xxinliu at amazon.com<mailto:xxinliu at amazon.com>>; 'hotspot-compiler-dev at openjdk.java.net<mailto: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

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><mailto: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><mailto:hohensee at amazon.com<mailto:hohensee at amazon.com>>>, "Liu, Xin" <xxinliu at amazon.com<mailto:xxinliu at amazon.com><mailto:xxinliu at amazon.com<mailto:xxinliu at amazon.com>>>, "'hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>'" <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net><mailto: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><mailto: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><mailto:xxinliu at amazon.com<mailto:xxinliu at amazon.com>>>; Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com><mailto:martin.doerr at sap.com<mailto:martin.doerr at sap.com>>>; 'hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>' <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net><mailto: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><mailto: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><mailto:martin.doerr at sap.com<mailto:martin.doerr at sap.com>>>, "Hohensee, Paul" <hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com<mailto:hohensee at amazon.com>>>, "'hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>'" <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net><mailto: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><mailto: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><mailto:xxinliu at amazon.com<mailto:xxinliu at amazon.com>>>, "Hohensee, Paul" <hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com<mailto:hohensee at amazon.com>>>, "'hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>'" <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net><mailto: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