RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Hohensee, Paul
hohensee at amazon.com
Fri Jul 20 18:37:13 UTC 2018
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