[15] RFR: 8237894: CTW: C1 compilation fails with assert(x->type()->tag() == f->type()->tag()) failed: should have same type

[Jamsheed C M]

Hi Tobias,

On 16/03/2020 16:14, Tobias Hartmann wrote:
> Hi Jamsheed,
>
> thanks for the detailed explanation. Looks good to me.
Thank you for the review.
>
> Some comments (no new webrev required):
> - c1_GraphBuilder.cpp:3780 Add brackets
> - c1_GraphBuilder.cpp:3792 Add brackets around if scope
> - c1_Instruction.hpp:839 Remove newline

Ok, sure.

Thanks and Best regards,

Jamsheed

>
> Best regards,
> Tobias
>
> On 13.03.20 07:24, Jamsheed C M wrote:
>> Hi,
>>
>> updated the test, as i had excluded main from compilation also added other cases described in the
>> comment.
>>
>> webrev updated in place.
>>
>> Best regards,
>>
>> Jamsheed
>>
>> On 12/03/2020 19:25, Jamsheed C M wrote:
>>> Hi Tobias, Aleksey,
>>>
>>> Did a little more research on this issue.
>>>
>>> Two cases+mitigation described below
>>>
>>> 1) Null based field access.
>>>     Value Numbering null based field access causes instructions to be eliminated across type/subtypes.
>>>     Declared type of these instructions are field type, so it being receiver causes problems to
>>> Type System.
>>>     To mitigate this issue, we hash declared type in addition to existing hashing.
>>> 2) Null based indexed access.
>>>     Value Numbering null based indexed access causes instructions to be eliminated across
>>> type/subtypes.
>>>     Element basic type in encoded in the access instruction, this causes problems to Type system.
>>>     Declared type of these instructions are null, so it being receiver doesn't cause any problem to
>>> Type System.
>>>     To mitigate this issue, we hash basic type in addition to existing hashing
>>>
>>> in addition i disable inlining if recv is constant null
>>>
>>> I have included case 1&2 in the newly added test.
>>>
>>> revised webrev: http://cr.openjdk.java.net/~jcm/8237894/webrev.02/
>>>
>>> testing:mach5 hs-tier1-5( running, link in jbs)
>>>
>>> request for review and feedback
>>>
>>> Thanks and Best regards,
>>>
>>> Jamsheed
>>>
>>> On 17/02/2020 21:18, Jamsheed C M wrote:
>>>> Hi Tobias,
>>>>
>>>> from further analysis, it seems it  is not good idea to do this fix either, as it  can still
>>>> cause arbitrary memory access an crashes even in production build in attempt resolving the method
>>>> for inlining.
>>>>
>>>> so, i think it is better to disable the value numbering for null constants and stop inlining over
>>>> it. assuming use of other use cases are limited and can have a performance penalty in those use
>>>> cases.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> On 17/02/2020 19:18, Jamsheed C M wrote:
>>>>> Hi Tobias,
>>>>>
>>>>> On 17/02/2020 18:34, Tobias Hartmann wrote:
>>>>>> On 17.02.20 13:34, Jamsheed C M wrote:
>>>>>>> i meant for null object field access or indexed access, the instruction can match across
>>>>>>> subtypes*
>>>>>>> and get eliminated. which can still put us in awkward situation if we try checking exact/declared
>>>>>>> types, which we normally don't do.
>>>>>>>
>>>>>>> *(as instruction types are erased types)
>>>>>> Right but it's not a problem with current code.
>>>>> i am guessing it, but i am not very sure from runtime side, if it has asserts checking recv type
>>>>> and declared type of current instruction (i.e resolve_invoke)
>>>>>
>>>>>>>>>> And given that, does your solution actually work with Object arrays?
>>>>>>>>> Yes, as we are asserting on elem instruction type for indexed access.
>>>>>>>> But isn't it the case that with your change, a null from an MyObject1[] load still has the
>>>>>>>> same hash
>>>>>>>> as a null from a MyObject2[] load because type()->tag() is the same? And wouldn't that still
>>>>>>>> trigger
>>>>>>>> the assert?
>>>>>>> it wouldn't, instruction types are erased types. so sub types all would match
>>>>>> Okay, I see. Thanks for the explanation.
>>>>>>
>>>>>> Webrev.01 seems reasonable to me but I think it would be good if someone with more C1 expertise
>>>>>> could take a look as well.
>>>>> sure.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Jamsheed
>>>>>
>>>>>> Thanks,
>>>>>> Tobias