RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Doerr, Martin
martin.doerr at sap.com
Tue Jul 17 09:31:25 UTC 2018
Hi Liu Xin,
I also believe that the -UseOnStackReplacement Label problem only exists on x86. But thanks for looking at other platforms (s390 is missing in your list).
About Compile::scratch_emit_size:
The concern is not about the is_branch case. Problematic could be n->emit(buf, this->regalloc()); while in_scratch_emit_size() is true.
The node specific emit function may use labels without binding.
If this assertion is desired, I'd expect at least an attempt to identify all bail out situations. And there should be some test coverage on all platforms. Unfortunately, I don't have time to help with that in the near future.
Best regards,
Martin
-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Liu Xin
Sent: Montag, 16. Juli 2018 23:10
To: hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Hi, List,
Could you review this new revision?
https://s3-us-west-2.amazonaws.com/openjdk-webrevs/jdk/label_bugfix/index.html <https://s3-us-west-2.amazonaws.com/openjdk-webrevs/jdk/label_bugfix/index.html>
i) I took a look at all architectures, arm/aarch64/ppc64/sparc/x86. I don’t understand all the assemblies, but I think they are guarded for UseOnStackReplacement
in templateTable_xxx.cpp ::branch(bool is_jsr, bool is_wide).
TemplateTable_arm.cpp is a little different. It explicitly binds it later.
if (!UseOnStackReplacement) {
__ bind(backedge_counter_overflow);
}
i) I checked the Compile::scratch_emit_size. It only uses the label fakeL for those MachBranch nodes.
Because fakeL will be bound to a trivial address if the nodes are MachBranch, It’s also safe for the assertion.
bool is_branch = n->is_MachBranch();
if (is_branch) {
MacroAssembler masm(&buf);
masm.bind(fakeL);
n->as_MachBranch()->save_label(&saveL, &save_bnum);
n->as_MachBranch()->label_set(&fakeL, 0);
}
Thanks,
—lx
> On Jul 16, 2018, at 1:30 AM, Doerr, Martin <martin.doerr at sap.com> wrote:
>
> Hi Liu Xin,
>
> thanks for changing.
>
> > The background of this Assertion is that our engineer used to spend many hour to trace down a corner case.
> > it's trivial if fastdebug/slowdebug stop and tell you immediately.
>
> I understand that. But an assertion should only get added when we are convinced that it won’t produce false positives.
> It’s very annoying if long running tests break due to an incorrect assertion after running many days.
>
> > I am curious about this "We also may generate code with the purpose to determine its size.".
> > Could you tell me where is it? it looks quite slow to get buffer size in this way.
>
> C2 Compiler does that in Compile::scratch_emit_size.
>
> Please note that I’ll be on vacation soon, so other people will have to review.
> Thanks again for fixing the -XX:-UseOnStackReplacement issue.
>
> Best regards,
> Martin
>
>
> From: Liu Xin [mailto:navy.xliu at gmail.com]
> Sent: Freitag, 13. Juli 2018 22:30
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8206075: add assertion for unbound assembler Labels for x86
>
> Hello, Martin,
>
> Thanks for reviewing it.
>
> I got your point. I made it "if (where != NULL) { jcc(cond, *where); }" and is running tests.
>
> The background of this Assertion is that our engineer used to spend many hour to trace down a corner case. it's trivial if fastdebug/slowdebug stop and tell you immediately.
>
> I am curious about this "We also may generate code with the purpose to determine its size.". Could you tell me where is it? it looks quite slow to get buffer size in this way.
>
> thanks,
> --lx
>
>
> On Fri, Jul 13, 2018 at 2:54 AM, Doerr, Martin <martin.doerr at sap.com <mailto:martin.doerr at sap.com>> wrote:
> Hi,
>
> thanks for fixing the issue in templateTable_x86. It looks correct.
> I think even better would be
> "UseOnStackReplacement ? &backedge_counter_overflow : NULL"
> and
> "if (where != NULL) { jcc(cond, *where); }" in interp_masm_x86.cpp.
> But I leave it up to you if you want to change it. I'm also ok with your version.
>
> I'm not convinced that the label assertion is reliable. There may be many more places in hotspot where we bail out having an unbound label. Running a few tests on x86 is by far not sufficient. The assertion may fire sporadically in large scenarios on some platforms.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net <mailto:hotspot-runtime-dev-bounces at openjdk.java.net>] On Behalf Of Liu Xin
> Sent: Donnerstag, 12. Juli 2018 22:51
> To: hotspot-runtime-dev at openjdk.java.net <mailto:hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR(S): 8206075: add assertion for unbound assembler Labels for x86
>
> Could you review this patch again?
>
> Revision #2.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8206075 <https://bugs.openjdk.java.net/browse/JDK-8206075> <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> <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
>
> PASSED: 75
> UNABLE_TO_RUN: 0
> KILLED: 0
> NA: 0
> FAILED: 0
> EXECUTED_WITH_FAILURE: 0
>
>
> Thanks,
> —lx
> > On Jul 11, 2018, at 10:35 AM, Liu Xin <navy.xliu at gmail.com <mailto: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.
> >
> > Thanks,
> > —lx
> >
> >> On Jul 11, 2018, at 7:49 AM, Hohensee, Paul <hohensee at amazon.com <mailto: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.
> >>
> >> Thanks,
> >>
> >> Paul
> >>
> >> On 7/11/18, 3:41 AM, "hotspot-runtime-dev on behalf of Doerr, Martin" <hotspot-runtime-dev-bounces at openjdk.java.net <mailto:hotspot-runtime-dev-bounces at openjdk.java.net> on behalf of martin.doerr at sap.com <mailto:martin.doerr at sap.com>> wrote:
> >>
> >> Hi,
> >>
> >> 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,
> >> Martin
> >>
> >>
> >> -----Original Message-----
> >> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net <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 <mailto:navy.xliu at gmail.com>>; hotspot-runtime-dev at openjdk.java.net <mailto: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:
> >>
> >> compiler/codegen/TestCharVect2.java
> >> compiler/c2/cr6340864/*
> >>
> >> Regards,
> >> Vladimir
> >>
> >> On 7/10/18 5:08 PM, Vladimir Kozlov wrote:
> >>> Fix looks reasonable. I will test it in our framework.
> >>>
> >>> Thanks,
> >>> Vladimir
> >>>
> >>> 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 <https://bugs.openjdk.java.net/browse/JDK-8206075>
> >>>> <https://bugs.openjdk.java.net/browse/JDK-8206075 <https://bugs.openjdk.java.net/browse/JDK-8206075>>
> >>>> CR: http://cr.openjdk.java.net/~phh/8206075/webrev.00/ <http://cr.openjdk.java.net/~phh/8206075/webrev.00/>
> >>>> <http://cr.openjdk.java.net/~phh/8206075/webrev.00/ <http://cr.openjdk.java.net/~phh/8206075/webrev.00/>>
> >>>> Problem:
> >>>> 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.
> >>>> Thanks,
> >>>> --lx
> >>>>
> >>
> >>
> >
>
More information about the hotspot-runtime-dev
mailing list