RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Jul 20 07:29:41 UTC 2018
Hi Liu,
Martin had put the patch into our testing queue.
All the platforms we build are fine.
This are: windows x86_64, linux: ppc64, ppc64le, x86_64, s390x,
aix ppc64, solaris sparcv9, mac.
Best regards,
Goetz.
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Liu Xin
> Sent: Freitag, 20. Juli 2018 09:16
> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8206075: add assertion for unbound assembler Labels for
> x86
>
> Hello, Vladimir,
> Could you run on other platform on behalf of Martin?
> I locally tested on x86_64. I hope the Reviewer can help me verify it works
> on other platforms.
>
>
> Furthermore, I am sure if we should add this additional patch.
> Label class is not POD, we should properly call constructor /destructor
> even though those labels are allocated on arena.
>
>
> thanks,
> --lx
>
> On Wed, Jul 18, 2018 at 4:07 AM, Doerr, Martin <martin.doerr at sap.com>
> wrote:
>
> > Hi Liu Xin,
> >
> >
> >
> > thanks for understanding my point and checking other places.
> >
> >
> >
> > The templateTable_x86.cpp was reviewed by me.
> >
> > I can’t review the label assertion before my vacation. If other reviewers
> > are convinced that the it is correct, ok.
> >
> >
> >
> > Would be great if somebody could assist with testing other platforms.
> >
> >
> >
> > Best regards,
> >
> > Martin
> >
> >
> >
> >
> >
> > *From:* Liu Xin [mailto:navy.xliu at gmail.com]
> > *Sent:* Dienstag, 17. Juli 2018 19:17
> >
> > *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
> >
> >
> >
> > Hi, Martin,
> >
> >
> >
> > Thank you for the feedback.
> >
> >
> >
> > I totally agree with you that we shouldn’t introduce false positive
> > assertion. Let’s insist on the high bar here.
> >
> > I browsed many sources in hotspot recently. Hotspot is the most monolithic
> > software I ever seen. I am glad to be directed by a guidance and clear
> > target.
> >
> >
> >
> > I think I dealt with c1 bailout case. This case triggers "codebuffer
> > overflow" in middle of c1 compilation.
> >
> > compiler/codegen/TestCharVect2.java
> >
> >
> >
> > I am still not sure about c2 bailout case. Let me try to make one.
> >
> >
> >
> > For case #2, I got what you concerned. Indeed, the generated ad_x86.cpp
> > contains many emits methods for MachNode. I will double-check if they
> could
> > leave unused labels.
> >
> >
> >
> > Thanks,
> >
> > —lx
> >
> >
> >
> >
> >
> > On Jul 16, 2018, at 2:09 PM, Liu Xin <navy.xliu at gmail.com> wrote:
> >
> >
> >
> > Hi, List,
> >
> >
> >
> > Could you review this new revision?
> >
> > 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 <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>
> > 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] On Behalf Of Liu Xin
> > Sent: Donnerstag, 12. Juli 2018 22:51
> > To: 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 <h
> > ttps://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
> >
> > 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> 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>
> > 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 on behalf of
> > 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] 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:
> > >>
> > >> 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>
> > >>>> CR: 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