RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Jul 20 18:46:59 UTC 2018
This looks good. I will sponsor it.
Thanks,
Vladimir
On 7/20/18 11:37 AM, Hohensee, Paul wrote:
> New webrev: http://cr.openjdk.java.net/~phh/8206075/webrev.01/
>
> Thanks,
>
> Paul
>
> On 7/20/18, 11:32 AM, "hotspot-runtime-dev on behalf of Vladimir Kozlov" <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of vladimir.kozlov at oracle.com> wrote:
>
> On 7/20/18 11:11 AM, Liu Xin wrote:
> > Thanks, Vladimir and Goetz. Could yo approve what you tested?
>
> I am fine with your latest changes but you need to post webrev on
> cr.openjdk. I will review it then.
>
> >
> >
> > For the patch, I think it's another story. I am *NOT* sure if we should
> > need it. It's about C++ object model. I feel hotspot is using C++ in
> > non-standard way. I am confusing about C++ in hotspot.
> > In regular C++ , we should manage the life cycle of objects carefully.
> >
> > If you take a look at usage of this macro, some non-pod classes don't
> > construct but use directly.
> > #define NEW_RESOURCE_ARRAY(type, size)\
> > (type*) resource_allocate_bytes((size) * sizeof(type))
> >
> > eg.
> > VMRegPair* out_regs = NEW_RESOURCE_ARRAY(VMRegPair, total_c_args);
> >
> > May I create a new RFR to enhance it?
> > I want to introduce a meta-programming template like boost's is_pod.
> > https://www.boost.org/doc/libs/1_44_0/libs/type_traits/doc/html/boost_typetraits/reference/is_pod.html
>
> Be careful. Hotspot have to be compiled by big variety of C++ compilers
> and not all of them support latest features.
>
> Regards,
> Vladimir
>
> >
> > NEW_RESOURCE_ARRAY should call constructors for those classes which are not
> > pod.
> >
> > thanks,
> > --lx
> >
> >
> >
> >
> > On Fri, Jul 20, 2018 at 9:18 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com
> >> wrote:
> >
> >> My testing also passed clean. I tested next patch:
> >>
> >> https://s3-us-west-2.amazonaws.com/openjdk-webrevs/jdk/
> >> label_bugfix/index.html
> >>
> >> Please, post it on cr.openjdk server for final review. We can't review and
> >> use patches from other places.
> >>
> >> Thanks,
> >> Vladimir
> >>
> >>
> >> On 7/20/18 12:29 AM, Lindenmaier, Goetz wrote:
> >>
> >>> 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