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

[Jamsheed C M]

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