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

[Christian Hagedorn]

Thank you Tobias for your review!

Best regards,
Christian

On 20.07.20 10:32, Tobias Hartmann wrote:
> +1
> 
> Best regards,
> Tobias
> 
> On 15.07.20 19:37, Vladimir Kozlov wrote:
>> Looks good.
>>
>> Thanks,
>> Vladimir K
>>
>> On 7/15/20 12:58 AM, Christian Hagedorn wrote:
>>> 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