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 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>
Sent: Donnerstag, 5. September 2019 21:29
To: Liu, Xin <xxinliu at amazon.com>; Doerr, Martin <martin.doerr at sap.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

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