memleak in CPP runtime: ParserRuleContext is not destroyed when a RecognitionError is thrown

[floor....@aimms.com]

Hi Mike, 

First of all: thank you so much for the cpp target of antlr4! 

I discovered a memleak caused by a (hidden) circular reference: 

The RecognitionException has a shared_ptr to the ParserRuleContext, but is stored in the ParserRuleContext, wrapped in the  std::exception_ptr.

This happens in the generated  lines in the XXParser in the catch section of the rule methods:

 _localctx->exception = std::current_exception();

I am under the impression that this leads quite generally to a memleak, but I would be happy to create a small reproducing example if you want. 

We are bailing on whatever error, so I have created a workaround by implementing a new BailErrorStrategy that sets the member of the Context to nullptr. 

...
 virtual void recover(antlr4::Parser *recognizer, std::exception_ptr e) override
    {
        auto ctx = recognizer->getContext();
        ctx->exception = nullptr;
        throw antlr4::ParseCancellationException();
    }
..

I would say that having the reference to the exception in the context is not necessary, as it is already provided in the report and recover methods of the errorhandler, but removing the reference would of course be a breaking change. 

Kind regards, 

Floor Goddijn

[Mike Lischke]

Hi Floor,

I discovered a memleak caused by a (hidden) circular reference: 

The RecognitionException has a shared_ptr to the ParserRuleContext, but is stored in the ParserRuleContext, wrapped in the  std::exception_ptr.

The good news is I simplified that part of memory handling a few days ago (https://github.com/DanMcLaughlin/antlr4/commit/4897bd857138e5ff2e4da2f611923f16c1e1bf07). The volatile part (which is what gets freed after each parse run) is now managed with a simple tracking list instead of passing around smart pointers, which again gives a bit better execution speed and simpler handling. The shared_ptr for the rule context in the recognition exception was btw causing a double free when you use the approach via SLL + LL parsing (and the bail out strategy), which I had to fix yesterday (I just pushed the changes to Github, https://github.com/DanMcLaughlin/antlr4/commit/014070de66de68e943bb159779372e63da5e4849).

I would say that having the reference to the exception in the context is not necessary, as it is already provided in the report and recover methods of the errorhandler, but removing the reference would of course be a breaking change. 

I'm also thinking since a while whether we really should keep the exception pointer around. What would be a good use case for this? Especially as it is kept in all parent contexts of the affected one for which the recognition exception was thrown. If nobody can come up with a good use case I might just remove that field.

Mike
-- 
www.soft-gems.net

[floor....@aimms.com]

Hi Mike, 

I have finally come around to use the newest source and test it and it is all fine, thank you. 

I do see a use case for the exception being available at the context.

If one does not want to implement any error listeners or error strategies but does wish to get some information at failure I think this is his only option. 

But as creating some error listener is easy enough, this seems not enough reason to keep it at all costs.

Regards, 

Floor

[Mike Lischke]

Hey Floor,

Thanks for your feedback. I agree with you here.

Mike
--
www.soft-gems.net