[16] RFR(S): 8247743: Segmentation fault in debug builds due to stack overflow in find_recur with deep graphs

[Christian Hagedorn]

Hi Vladimir

On 14.07.20 20:46, Vladimir Kozlov wrote:
> Can you move next up to where other small find*() methods are defined?:
> 
> +Node* Node::find_ctrl(int idx) {
> +  return find(idx, true);
>   }
> 
> Also add '// not PRODUCT' comment to #endif for #ifndef PRODUCT. It is 
> hard to find where this not product code ends.
> 
> Looks good otherwise.

Thanks, I added these changes in a new webrev:
http://cr.openjdk.java.net/~chagedorn/8247743/webrev.02/

Best regards,
Christian


> Thanks,
> Vladimir
> 
> On 7/14/20 2:54 AM, Christian Hagedorn wrote:
>> Hi Vladimir
>>
>> On 13.07.20 19:43, Vladimir Kozlov wrote:
>>> Node::find_ctrl() is used during debugging when you want to print and 
>>> look on only control nodes.
>>> We have several such methods which are only used in debugger.
>>
>> I see, I restored this method and changed Node::find() accordingly. I 
>> additionally added two find_ctrl() methods to make it easier to call 
>> it from a debugger (as already present for find_node()).
>>
>>> I suggest to store old_arena() in local var and pass into 
>>> add_to_worklist().
>>>
>>> You can make add_to_worklist() static since you pass node as argument.
>>
>> Okay. I updated this and the change above in a new webrev:
>> http://cr.openjdk.java.net/~chagedorn/8247743/webrev.01/
>>
>> Best regards,
>> Christian
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/13/20 3:09 AM, Christian Hagedorn wrote:
>>>> Ping - could anyone review it, please? Thanks!
>>>>
>>>> Best regards,
>>>> Christian
>>>>
>>>> On 02.07.20 09:33, Christian Hagedorn wrote:
>>>>> Hi
>>>>>
>>>>> Please review the following patch:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8247743
>>>>> http://cr.openjdk.java.net/~chagedorn/8247743/webrev.00/
>>>>>
>>>>> The testcase creates a deep graph with a lot of nodes on a chain. 
>>>>> When running with the specified test flags, it recursively calls 
>>>>> Node::find_recur() for each node discovered which eventually 
>>>>> results in a segmentation fault due to a stack overflow (around 
>>>>> 10000 calls due to such a long chain of nodes). The fix just 
>>>>> converts the recursive algorithm into an iterative one to avoid a 
>>>>> segmentation fault. This is similar to JDK-8246203 [1].
>>>>>
>>>>> I additionally removed Node::find_ctrl() and its special handling 
>>>>> in the algorithm since it is not used.
>>>>>
>>>>> There is actually another problem with the recursive version. When 
>>>>> running the testcase without 
>>>>> -XX:CompileOnly=compiler/c2/TestFindNode, it will spin forever 
>>>>> inside [2] because there is a debug_orig node cycle and the loop 
>>>>> does not break based on the debug_orig nodes being visited. This is 
>>>>> also fixed in the patch.
>>>>>
>>>>> Thank you!
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8246203
>>>>> [2] 
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/e2622818f0bd/src/hotspot/share/opto/node.cpp#l1589 
>>>>>