[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, 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