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

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