RFR(S): 8206075: add assertion for unbound assembler Labels for x86
Liu Xin
navy.xliu at gmail.com
Fri Jul 20 18:11:50 UTC 2018
Thanks, Vladimir and Goetz. Could yo approve what you tested?
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
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