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

[Tobias Hartmann]

Hi Jamsheed,

thanks for the detailed explanation. Looks good to me.

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

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