Exception handling in selection parsing
Currently, very few C++ exceptions are generated during selection parsing (and even those only for internal errors or out-of-memory conditions in small allocations). However, if/when larger parts of the code (e.g., in #652) are converted to C++, more cases will come up. Currently, any exception that passes through parser.cpp (source file generated by Bison) can cause some memory leaks. The code generated by Flex (scanner.cpp) is less of an issue, since it calls only a few functions, but it should also be analyzed.Things to consider:
- One solution is to catch all exceptions, either in the parser, or in each function it calls, and log their contents into the MessageStringCollector that is already now used to collect error messages. Code that calls the parser then throws a single exception based on the error messages. If this alternative is chosen, there should be a common pattern of handling all relevant exceptions. The code that throws the exception currently only throws a hard-coded exception type, but this is probably not sufficient in the long run.
- Newer versions of Bison support push parsers, which may allow passing exceptions through the parser and then destroying the parser data structure when the exception is caught, but this needs to be analyzed. The push parser would also simplify the code otherwise, so this alternative would be nice to implement in any case.
- Bison can also generate C++ code for the parser, but quick googling seems to suggest that this may not handle exceptions very well.
Misc. exception-related fixes.
- Catch exceptions by const reference (probably has no effect on
generated code, but is according to some best practices).
- Remove unnecessary cleanup code from analysismodule.cpp that could
actually trigger asserts.
- Remove currently unnecessary cleanup code from parsetree.cpp and
create issue #880 to discuss a more general solution.
Use push parser for selection parsing.
The push parser is new in Bison 2.4 (current parser.cpp files generated
with 2.5), and has two benefits in our case:
- Clearer control flow, since the interactive prompt loop is now outside
the parser, so it doesn't need to behave like a state machine.
- Allows better exception handling: since it is possible to directly
return from Flex actions, we should also be able to throw exceptions.
But without this change, they would go through the generated part of
the Bison parser, causing memory leaks since the code is generated for
C. With this change, the Flex scanner is called directly from C++
code, eliminating this problem.
Since the Bison and Flex parsers are now decoupled, it should also be
possible to clarify the code further, by having a separate data
structure for parser-specific data instead of using yyscan_t. For now,
kept the changes to a minimum.
Related to #880.
Make the selection parser exception-proof.
The parser now catches and stores any exception that occurs during
parsing, terminates the parser cleanly, and rethrows the exception.
The functions called from the parser are still not exception-safe, and
would require quite some work to make so, because they are code written
originally in C and do a lot of dynamic memory allocation, returning
some of the allocated memory to the caller. But the generated code
should now work with exceptions.
Remove most uses of t_selelem::refcount.
This is a prerequisite for using smart pointers to manage the selection
evaluation tree, as it's difficult to keep a similar reference count
up-to-date and (or use pointer reference counts for the purposes it is
currently used for).
Use smart pointers to manage the selection tree.
Changed t_selelem to gmx::SelectionTreeElement and t_selelem * to
gmx::SelectionTreeElementPointer (or a const reference). The latter is
a boost::shared_ptr because there may be multiple references to some
elements in the evaluation tree. Some exceptions to the above rule are
present, because it was not straightforward to change those parts.
Prerequisite for using exceptions throughout the selection code, which
in turn is a prerequisite for many tasks that would introduce more C++
There are some parts that are not yet exception-safe, but may throw
std::bad_alloc as the result of these changes. Will fix those in
C++ classes for selection parser symbol table.
Convert the implementation of selection parser symbol table to use C++
- Removes a few exception safety issues that were present in the
initialization, as well as direct prints to stderr.
- Documentation now follows the new Doxygen layout.
- Removes some underscore-prefixed global names.
C++ classes for selection parser parameters.
Converted t_selexpr_param to gmx::SelectionParserParameter and
introduced smart pointers and standard containers to manage all memory
related to these objects. Solves several exception safety issues in the
Handle SelectionParserParameter as a value type.
Now that SelectionParserValue is also a C++ object, it is
straightforward to treat SelectionParserParameter as a (movable, but not
copyable) value type instead of storing it through a smart pointer in a
container. Leads to more consistent and less complex code.
#1 Updated by Teemu Murtola over 7 years ago
- Status changed from New to In Progress
- Assignee set to Teemu Murtola
- Target version set to 5.0
After looking at the alternatives a bit, I think that the best alternative is to switch to the push parser, and then use macros like BEGIN_ACTION and END_ACTION in each parser action to handle exceptions uniformly. An attractive alternative is to catch all exceptions in END_ACTION and call a handler function. This function could then determine whether it is possible to continue parsing (i.e., to issue an yyerror in the parser), or to terminate immediately (with YYABORT in the parser). To rethrow the exception in the latter case, boost::current_exception could be used.