RFR(S): 8235762: JVM crash in SWPointer during C2 compilation

[Christian Hagedorn]

Hi Felix

Thanks for explaining. Following your analysis with the provided test 
case, orig_msize is 0 in the end. Can you also provide a test case or 
show an example which covers the else case in this test:

  717             if (orig_msize == 0) {
  718               best_align_to_mem_ref = memops.at(max_idx)->as_Mem();
  719             } else {
  720               for (uint i = 0; i < orig_msize; i++) {
  721                 memops.remove(0);
  722               }
  723               best_align_to_mem_ref = find_align_to_ref(memops, 
max_idx);
  724               assert(best_align_to_mem_ref == NULL, "sanity");
  725               best_align_to_mem_ref = memops.at(max_idx)->as_Mem();
  726             }

Best regards,
Christian


On 12.12.19 07:24, Yangfei (Felix) wrote:
> Hi,
> 
>    I have created a webrev for the patch: http://cr.openjdk.java.net/~fyang/8235762/webrev.00/
>    Tested tier1-3 with both aarch64 and x86_64 linux release build.
>    Newly added test case fail without the patch and pass with the patch.
> 
> Thanks,
> Felix
> 
>> -----Original Message-----
>> From: Yangfei (Felix)
>> Sent: Wednesday, December 11, 2019 11:12 PM
>> To: 'Christian Hagedorn' <christian.hagedorn at oracle.com>; Tobias Hartmann
>> <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
>> Subject: RE: RFR(S): 8235762: JVM crash in SWPointer during C2 compilation
>>
>> Hi Christian,
>>
>> Thanks for the suggestions. Comments inlined.
>>
>>> -----Original Message-----
>>> From: Christian Hagedorn [mailto:christian.hagedorn at oracle.com]
>>> Sent: Wednesday, December 11, 2019 10:54 PM
>>> To: Yangfei (Felix) <felix.yang at huawei.com>; Tobias Hartmann
>>> <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
>>> Subject: Re: RFR(S): 8235762: JVM crash in SWPointer during C2
>>> compilation
>>>
>>> Hi Felix
>>>
>>> Thanks for working on that. Your fix also seems to work for JDK-8235700.
>>> I closed that one as a duplicate of yours.
>>>
>>>>>> +              for (int i = 0; i < orig_msize; i++) {
>>>
>>> Should be uint since orig_msize is a uint
>>
>> -- Yes, will modify accordingly when I am preparing webrev.
>>
>>>
>>>>>> +              best_align_to_mem_ref = find_align_to_ref(memops,
>>>>> max_idx);
>>>>>> +              assert(best_align_to_mem_ref == NULL, "sanity");
>>>
>>> You can merge these two lines together into
>>> assert(find_align_to_ref(memops,
>>> max_idx) == NULL, "sanity"); since the call belongs to the sanity
>>> check. Or just surround it by a #ifdef ASSERT.
>>
>> -- The purpose of line 721 here is to calculate the max_idx.
>>    So I don't think it's suitable to treat this line as assertion logic.
>>
>>>>>> +  idx = max_idx;
>>>
>>> Is max_idx always guaranteed to be valid and not -1 when accessing it later?
>>
>> -- Yes, I think so.
>>    When memops is not empty and the memory ops in memops are not
>> comparable, find_align_to_ref will always sets its max_idx.
>>
>> Thanks,
>> Felix