RFR(S): 8206075: add assertion for unbound assembler Labels for x86
navy.xliu at gmail.com
Thu Jul 12 20:51:12 UTC 2018
Could you review this patch again?
Bug: https://bugs.openjdk.java.net/browse/JDK-8206075 <https://bugs.openjdk.java.net/browse/JDK-8206075>
CR: https://s3-us-west-2.amazonaws.com/openjdk-webrevs/openjdk8u/webrev/index.html <https://s3-us-west-2.amazonaws.com/openjdk-webrevs/openjdk8u/webrev/index.html>
The idea is simple. I just reset the problematic label when c1 compilation bailout happen.
I manually ran tier1 on my laptop. it can pass all of them.
Paul help me submit the patch to submit and here is the run result.
Build Details: 2018-07-12-1736388.hohensee.source
0 Failed Tests
Mach5 Tasks Results Summary
> On Jul 11, 2018, at 10:35 AM, Liu Xin <navy.xliu at gmail.com> wrote:
> Thank you for your reviews. Indeed, I didn’t deal with bailout situation. "compiler/codegen/TestCharVect2.java” is the case of codeBuffer overflow and leave a unbound label behind.
> I made another revision. I will run tests thoroughly.
>> On Jul 11, 2018, at 7:49 AM, Hohensee, Paul <hohensee at amazon.com> wrote:
>> Imo it's still good hygiene to require that Labels be bound if they're used, even if the generated code will never be executed. E.g., code that generates code for sizing purposes may be repurposed to generate executable code, in which case an unbound label may be a lurking bug. Also, I'm unaware (I may be corrected!) of any situation where bailing out happens in such a way as to both leave a Label unbound and execute its destructor. Even if there are, I'd say that'd be indicative of another real problem, such as code buffer overflow, so no harm would result.
>> On 7/11/18, 3:41 AM, "hotspot-runtime-dev on behalf of Doerr, Martin" <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of martin.doerr at sap.com> wrote:
>> I think the idea is good, but doesn't work in all cases.
>> We may bail out from code generation and discard the generated code leaving the label unbound.
>> We also may generate code with the purpose to determine its size. We don't need to bind labels because the code will never get executed.
>> Best regards,
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
>> Sent: Mittwoch, 11. Juli 2018 03:34
>> To: Liu Xin <navy.xliu at gmail.com>; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(S): 8206075: add assertion for unbound assembler Labels for x86
>> I hit new assert in few other tests:
>> On 7/10/18 5:08 PM, Vladimir Kozlov wrote:
>>> Fix looks reasonable. I will test it in our framework.
>>> On 7/10/18 9:50 AM, Liu Xin wrote:
>>>> Hi, Community,
>>>> Could you please review this small patch?
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8206075
>>>> CR: http://cr.openjdk.java.net/~phh/8206075/webrev.00/
>>>> X86-32/64 will leave an unbound label if UseOnStackReplacement is OFF.
>>>> This patch align up x86 with other architectures(ppc, arm).
>>>> Add an assertion to the destructor of Label. It will be wiped out in release build.
>>>> Previously, hotspot cannot pass this test with assertion on x86-64.
>>>> make run-test TEST=test/hotspot/jtreg/compiler/c1/Test7090976.java
>>>> If this CR is approved, Paul Hohensee will push it.
More information about the hotspot-runtime-dev