RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Liu Xin
navy.xliu at gmail.com
Fri Jul 20 18:49:00 UTC 2018
Cool. thanks.
On Fri, Jul 20, 2018 at 11:46 AM, Vladimir Kozlov <
vladimir.kozlov at oracle.com> wrote:
> 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