[15] RFR(S): 8246203: Segmentation fault in verification due to stack overflow with -XX:+VerifyIterativeGVN

[Christian Hagedorn]

Thank you Vladimir for your review!

Best regards,
Christian

On 09.06.20 19:54, Vladimir Kozlov wrote:
> Good.
> 
> Thanks,
> Vladimir
> 
> On 6/9/20 10:32 AM, Christian Hagedorn wrote:
>> Hi Vladimir
>>
>> On 09.06.20 18:54, Vladimir Kozlov wrote:
>>> I think the check should be 'verify_depth > 0' because verify_depth 
>>> can be negative:
>>>
>>> +  verify_depth--; // Visiting the first node on depth 1
>>> +  bool add_to_worklist = verify_depth != 0;
>>>
>>> Or it is intentional for negative value to visit all nodes? Then it 
>>> needs comment.
>>
>> Yes, with negative values it visit all nodes. There is a comment about 
>> it above:
>>
>> 2155 // Verify all nodes if verify_depth is negative
>> 2156 void Node::verify(Node* n, int verify_depth) {
>>
>> But maybe I should add another comment for add_to_worklist as well to 
>> make it more clear.
>>
>>> In such case you need restore verify_depth == 0 check to return 
>>> otherwise with 0 the code will work as with negative value:
>>>    if (verify_depth == 0) {
>>>      return;
>>>    }
>>>    bool add_to_worklist = true;
>>>
>>> Or may be use assert(verify_depth != 0, "sanity") instead of check.
>>
>> I like the solution of an assert. I added it at the start of the 
>> method together with the additional comment in a new webrev. It only 
>> initializes add_to_worklist with false if verify() is called with 
>> verify_depth = 1.
>>
>> http://cr.openjdk.java.net/~chagedorn/8246203/webrev.01/
>>
>> Best regards,
>> Christian
>>
>>
>>> On 6/9/20 8:26 AM, Christian Hagedorn wrote:
>>>> Hi
>>>>
>>>> Please review the following patch:
>>>> https://bugs.openjdk.java.net/browse/JDK-8246203
>>>> http://cr.openjdk.java.net/~chagedorn/8246203/webrev.00/
>>>>
>>>> The testcase creates a deep graph with a lot of nodes on a chain. 
>>>> When running with -XX:+VerifyIterativeGVN, it recursively calls 
>>>> Node::verify_recur() for each input node discovered which eventually 
>>>> results in a segmentation fault due to a stack overflow (around 
>>>> 10000 recursive 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.
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>> Christian