RFR(S): 8206075: add assertion for unbound assembler Labels for x86

Fri Jul 20 07:16:05 UTC 2018

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
> >>>>
> >>
> >>
> >
>
>
>
>
>