Project

General

Profile

Task #880

Exception handling in selection parsing

Added by Teemu Murtola over 7 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
selections
Target version:
Difficulty:
uncategorized
Close

Description

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.

Related issues

Related to GROMACS - Task #655: Improve selection error reporting / switch to exceptionsClosed01/09/2011
Blocks GROMACS - Task #652: Change selection method implementation to use C++Blocked, need info

Associated revisions

Revision faf24658 (diff)
Added by Teemu Murtola over 7 years ago

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.

Change-Id: I91bae217f51fffd9524d7b9598edf8bd6fc43b2f

Revision 016d8bd9 (diff)
Added by Teemu Murtola over 7 years ago

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.

Change-Id: I43ea11561d3dd0577dd95eae3794ca621953d038

Revision e51f0c8c (diff)
Added by Teemu Murtola over 7 years ago

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.

Fixes #880.

Change-Id: I2d534d106a6fe20d949b3f24b0d7f9980c545fcf

Revision 6e877567 (diff)
Added by Teemu Murtola about 7 years ago

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).

Related to #655 and #880.

Change-Id: I9e6ae98067f8fe96d39aea6df076e2675ecb8a1f

Revision ddaf0f3b (diff)
Added by Teemu Murtola about 7 years ago

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.

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
subsequent commits.

Related to #655 and #880.

Change-Id: Ib36c08bac1ab2c9818604e45e2eb14a90069efdb

Revision 6e52b492 (diff)
Added by Teemu Murtola about 7 years ago

C++ classes for selection parser symbol table.

Convert the implementation of selection parser symbol table to use C++
classes.
- 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.

Related to #655 and #880.

Change-Id: Iecb690fa2e413c9dc487aacffac62a248ea3867a

Revision 85b98012 (diff)
Added by Teemu Murtola about 7 years ago

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
selection parser.

Related to #655 and #880.

Change-Id: Ia4c477f1313b49cd1da3b0407bec6e7a1a8b9296

Revision 3e234326 (diff)
Added by Teemu Murtola about 7 years ago

C++ classes for selection parser values.

Converted t_selexpr_value to gmx::SelectionParserValue and introduced
smart pointers and standard containers to manage all memory related to
these objects.

Related to #655 and #880.

Change-Id: Ide2d9db80e89f36c94dccc25085d49959e4115a8

Revision 64a06b73 (diff)
Added by Teemu Murtola about 7 years ago

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.

Related to #655 and #880.

Change-Id: I377bac05b1bd990ad95b38887f0f7b825bf0a805

Revision 7ce47fc7 (diff)
Added by Teemu Murtola about 7 years ago

Remove remaining GMX_ERROR uses.

Replaced those still left in selection parsing code by exceptions or
assertions.

Related to #655 and #880.

Change-Id: I89bff0c03eb15176e699529888aaae532f0c4138

History

#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.

#2 Updated by Teemu Murtola over 7 years ago

  • Status changed from In Progress to Closed

The generated code should now be exception-safe. It's a separate task to make the rest of the parser exception-safe, though...

#3 Updated by Teemu Murtola over 5 years ago

  • Project changed from Next-generation analysis tools to GROMACS
  • Category set to selections

Also available in: Atom PDF