RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Jul 20 16:18:52 UTC 2018
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