Parser: Fix a minor memory leak due to exception

WeiqunZhang · WeiqunZhang · commit feaebf7803b0 · 2025-10-15T12:25:08.000-07:00
* If an exception is thrown, we need to delete the memory. * Remove unused functions. * Minor reorganization. * Minor optimization of constant folding. Sync w/ amrex AMReX-Codes/amrex#4442.
diff --git a/Src/amrexpr_Parser.cpp b/Src/amrexpr_Parser.cpp
@@ -30,6 +30,8 @@ Parser::define (std::string const& func_body)
         try {
             amrexpr_parserparse();
         } catch (const std::runtime_error& e) {
+            amrexpr_parser_delete_buffer(buffer); // delete buffer allocated by bison
+            amrexpr_parser_delete_ptrs();         // delete ptrs allocated by amrexpr
             throw std::runtime_error(std::string(e.what()) + " in Parser expression \""
                                      + m_data->m_expression + "\"");
         }
diff --git a/Src/amrexpr_Parser_Y.H b/Src/amrexpr_Parser_Y.H
@@ -263,9 +263,9 @@ struct amrexpr_parser {
 
 struct amrexpr_parser* amrexpr_parser_new ();
 void amrexpr_parser_delete (struct amrexpr_parser* parser);
+void amrexpr_parser_delete_ptrs ();
 
-struct amrexpr_parser* parser_dup (struct amrexpr_parser* source);
-struct parser_node* parser_ast_dup (struct amrexpr_parser* parser, struct parser_node* node, int move);
+struct parser_node* parser_ast_dup (struct amrexpr_parser* parser, struct parser_node* node);
 
 void parser_regvar (struct amrexpr_parser* parser, char const* name, int i);
 void parser_setconst (struct amrexpr_parser* parser, char const* name, double c);
@@ -274,7 +274,7 @@ std::set<std::string> parser_get_symbols (struct amrexpr_parser* parser);
 int parser_depth (struct amrexpr_parser* parser);
 
 /* We need to walk the tree in these functions */
-void parser_ast_optimize (struct parser_node* node, std::map<std::string,double>& local_consts);
+void parser_ast_optimize (struct parser_node*& node, std::map<std::string,double>& local_consts);
 std::size_t parser_ast_size (struct parser_node* node);
 void parser_ast_print (struct parser_node* node, std::string const& space, std::ostream& printer);
 void parser_ast_regvar (struct parser_node* node, char const* name, int i);
@@ -288,6 +288,7 @@ void parser_ast_sort (struct parser_node* node);
 double parser_get_number (struct parser_node* node);
 void parser_set_number (struct parser_node* node, double v);
 bool parser_node_equal (struct parser_node* a, struct parser_node* b);
+bool parser_same_symbol (struct parser_node* a, struct parser_node* b);
 /*******************************************************************/
 
 template <typename T>
diff --git a/Src/amrexpr_Parser_Y.cpp b/Src/amrexpr_Parser_Y.cpp
@@ -7,6 +7,7 @@
 #include <cstdarg>
 #include <iostream>
 #include <stdexcept>
+#include <vector>
 
 void
 amrexpr_parsererror (char const *s, ...)
@@ -23,6 +24,7 @@ namespace amrexpr {
 
 namespace {
     struct parser_node* parser_root = nullptr;
+    std::vector<void*>  parser_ptrs;
 }
 
 // This is called by a bison rule to store the original AST in a static variable.
@@ -37,17 +39,20 @@ parser_makesymbol (char* name)
 {
     // We allocate more than enough space so that late we can turn parser_symbol
     // into into parser_node if necessary.
-    auto *symbol = (struct parser_symbol*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *symbol = (struct parser_symbol*) parser_ptrs.back(); // NOLINT
     symbol->type = PARSER_SYMBOL;
     symbol->name = strdup(name);
+    parser_ptrs.push_back(symbol->name);
     symbol->ip = -1;
     return symbol;
 }
 
 struct parser_node*
 parser_newnode (enum parser_node_t type, struct parser_node* l, struct parser_node* r)
 {
-    auto *tmp = (struct parser_node*) std::malloc(sizeof(struct parser_node));
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *tmp = (struct parser_node*) parser_ptrs.back();
     if (type == PARSER_SUB) {
         tmp->type = PARSER_ADD;
         tmp->l = l;
@@ -63,7 +68,8 @@ parser_newnode (enum parser_node_t type, struct parser_node* l, struct parser_no
 struct parser_node*
 parser_newneg (struct parser_node* n)
 {
-    auto *tmp = (struct parser_node*) std::malloc(sizeof(struct parser_node));
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *tmp = (struct parser_node*) parser_ptrs.back();
     tmp->type = PARSER_MUL;
     tmp->l = parser_newnumber(-1.0);
     tmp->r = n;
@@ -75,7 +81,8 @@ parser_newnumber (double d)
 {
     // We allocate more than enough space so that late we can turn parser_number
     // into into parser_node if necessary.
-    auto *r = (struct parser_number*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *r = (struct parser_number*) parser_ptrs.back(); // NOLINT
     r->type = PARSER_NUMBER;
     r->value = d;
     return (struct parser_node*) r;
@@ -90,7 +97,8 @@ parser_newsymbol (struct parser_symbol* symbol)
 struct parser_node*
 parser_newf1 (enum parser_f1_t ftype, struct parser_node* l)
 {
-    auto *tmp = (struct parser_f1*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *tmp = (struct parser_f1*) parser_ptrs.back(); // NOLINT
     tmp->type = PARSER_F1;
     tmp->l = l;
     tmp->ftype = ftype;
@@ -100,7 +108,8 @@ parser_newf1 (enum parser_f1_t ftype, struct parser_node* l)
 struct parser_node*
 parser_newf2 (enum parser_f2_t ftype, struct parser_node* l, struct parser_node* r)
 {
-    auto *tmp = (struct parser_f2*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *tmp = (struct parser_f2*) parser_ptrs.back(); // NOLINT
     tmp->type = PARSER_F2;
     tmp->l = l;
     tmp->r = r;
@@ -112,7 +121,8 @@ struct parser_node*
 parser_newf3 (enum parser_f3_t ftype, struct parser_node* n1, struct parser_node* n2,
               struct parser_node* n3)
 {
-    auto *tmp = (struct parser_f3*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *tmp = (struct parser_f3*) parser_ptrs.back(); // NOLINT
     tmp->type = PARSER_F3;
     tmp->n1 = n1;
     tmp->n2 = n2;
@@ -124,7 +134,8 @@ parser_newf3 (enum parser_f3_t ftype, struct parser_node* n1, struct parser_node
 struct parser_node*
 parser_newassign (struct parser_symbol* sym, struct parser_node* v)
 {
-    auto *r = (struct parser_assign*) std::malloc(sizeof(struct parser_node)); // NOLINT
+    parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+    auto *r = (struct parser_assign*) parser_ptrs.back(); // NOLINT
     r->type = PARSER_ASSIGN;
     r->s = sym;
     r->v = v;
@@ -137,7 +148,8 @@ parser_newlist (struct parser_node* nl, struct parser_node* nr)
     if (nr == nullptr) {
         return nl;
     } else {
-        auto *r = (struct parser_node*) std::malloc(sizeof(struct parser_node));
+        parser_ptrs.push_back(std::malloc(sizeof(struct parser_node)));
+        auto *r = (struct parser_node*) parser_ptrs.back();
         r->type = PARSER_LIST;
         r->l = nl;
         r->r = nr;
@@ -156,7 +168,9 @@ amrexpr_parser_new ()
     my_parser->p_root = std::malloc(my_parser->sz_mempool);
     my_parser->p_free = my_parser->p_root;
 
-    my_parser->ast = parser_ast_dup(my_parser, parser_root, 1); /* 1: free the source parser_root */
+    my_parser->ast = parser_ast_dup(my_parser, parser_root);
+
+    amrexpr_parser_delete_ptrs();
 
     if ((char*)my_parser->p_root + my_parser->sz_mempool != (char*)my_parser->p_free) {
         throw std::runtime_error("amrexpr_parser_new: error in memory size");
@@ -176,6 +190,15 @@ amrexpr_parser_delete (struct amrexpr_parser* parser)
     std::free(parser);
 }
 
+void
+amrexpr_parser_delete_ptrs ()
+{
+    for (auto* p : parser_ptrs) {
+        std::free(p);
+    }
+    parser_ptrs.clear();
+}
+
 namespace {
 
 std::size_t
@@ -197,19 +220,6 @@ parser_allocate (struct amrexpr_parser* my_parser, std::size_t N)
 
 }
 
-struct amrexpr_parser*
-parser_dup (struct amrexpr_parser* source)
-{
-    auto *dest = (struct amrexpr_parser*) std::malloc(sizeof(struct amrexpr_parser));
-    dest->sz_mempool = source->sz_mempool;
-    dest->p_root = std::malloc(dest->sz_mempool);
-    dest->p_free = dest->p_root;
-
-    dest->ast = parser_ast_dup(dest, source->ast, 0); /* 0: don't free the source */
-
-    return dest;
-}
-
 std::size_t
 parser_ast_size (struct parser_node* node)
 {
@@ -260,7 +270,7 @@ parser_ast_size (struct parser_node* node)
 }
 
 struct parser_node*
-parser_ast_dup (struct amrexpr_parser* my_parser, struct parser_node* node, int move)
+parser_ast_dup (struct amrexpr_parser* my_parser, struct parser_node* node)
 {
     void* result = nullptr;
 
@@ -288,55 +298,45 @@ parser_ast_dup (struct amrexpr_parser* my_parser, struct parser_node* node, int
     case PARSER_LIST:
         result = parser_allocate(my_parser, sizeof(struct parser_node));
         std::memcpy(result, node          , sizeof(struct parser_node));
-        ((struct parser_node*)result)->l = parser_ast_dup(my_parser, node->l, move);
-        ((struct parser_node*)result)->r = parser_ast_dup(my_parser, node->r, move);
+        ((struct parser_node*)result)->l = parser_ast_dup(my_parser, node->l);
+        ((struct parser_node*)result)->r = parser_ast_dup(my_parser, node->r);
         break;
     case PARSER_F1:
         result = parser_allocate(my_parser, sizeof(struct parser_node));
         std::memcpy(result, node          , sizeof(struct parser_f1));
         ((struct parser_f1*)result)->l = parser_ast_dup(my_parser,
-                                                 ((struct parser_f1*)node)->l, move);
+                                                 ((struct parser_f1*)node)->l);
         break;
     case PARSER_F2:
         result = parser_allocate(my_parser, sizeof(struct parser_node));
         std::memcpy(result, node          , sizeof(struct parser_f2));
         ((struct parser_f2*)result)->l = parser_ast_dup(my_parser,
-                                                 ((struct parser_f2*)node)->l, move);
+                                                 ((struct parser_f2*)node)->l);
         ((struct parser_f2*)result)->r = parser_ast_dup(my_parser,
-                                                 ((struct parser_f2*)node)->r, move);
+                                                 ((struct parser_f2*)node)->r);
         break;
     case PARSER_F3:
         result = parser_allocate(my_parser, sizeof(struct parser_node));
         std::memcpy(result, node          , sizeof(struct parser_f3));
         ((struct parser_f3*)result)->n1 = parser_ast_dup(my_parser,
-                                                 ((struct parser_f3*)node)->n1, move);
+                                                 ((struct parser_f3*)node)->n1);
         ((struct parser_f3*)result)->n2 = parser_ast_dup(my_parser,
-                                                 ((struct parser_f3*)node)->n2, move);
+                                                 ((struct parser_f3*)node)->n2);
         ((struct parser_f3*)result)->n3 = parser_ast_dup(my_parser,
-                                                 ((struct parser_f3*)node)->n3, move);
+                                                 ((struct parser_f3*)node)->n3);
         break;
     case PARSER_ASSIGN:
         result = parser_allocate(my_parser, sizeof(struct parser_node));
         std::memcpy(result, node          , sizeof(struct parser_assign));
         ((struct parser_assign*)result)->s = (struct parser_symbol*)
             parser_ast_dup(my_parser, (struct parser_node*)
-                                                (((struct parser_assign*)node)->s), move);
+                                                (((struct parser_assign*)node)->s));
         ((struct parser_assign*)result)->v = parser_ast_dup(my_parser,
-                                                 ((struct parser_assign*)node)->v, move);
+                                                 ((struct parser_assign*)node)->v);
         break;
     default:
         throw std::runtime_error("parser_ast_dup: unknown node type " + std::to_string(node->type));
     }
-    if (move) {
-        /* Note that we only do this on the original AST.  We do not
-         * need to call free for AST stored in amrexpr_parser because the
-         * memory is not allocated with std::malloc directly.
-         */
-        if (node->type == PARSER_SYMBOL) {
-            std::free(((struct parser_symbol*)node)->name);
-        }
-        std::free((void*)node);
-    }
     return (struct parser_node*)result;
 }
 
@@ -347,14 +347,6 @@ namespace {
         return ((struct parser_symbol*)node)->name;
     }
 
-    bool parser_same_symbol (struct parser_node* a, struct parser_node* b)
-    {
-        return (a->type == PARSER_SYMBOL)
-            && (b->type == PARSER_SYMBOL)
-            && (std::strcmp(((struct parser_symbol*)a)->name,
-                            ((struct parser_symbol*)b)->name) == 0);
-    }
-
     bool is_add_combinable (struct parser_node* a, struct parser_node*b)
     {
         if ((a->type == PARSER_NUMBER) &&
@@ -608,6 +600,14 @@ namespace {
     }
 }
 
+bool parser_same_symbol (struct parser_node* a, struct parser_node* b)
+{
+    return (a->type == PARSER_SYMBOL)
+        && (b->type == PARSER_SYMBOL)
+        && (std::strcmp(((struct parser_symbol*)a)->name,
+                        ((struct parser_symbol*)b)->name) == 0);
+}
+
 bool parser_node_equal (struct parser_node* a, struct parser_node* b)
 {
     if (a->type != b->type) { return false; }
@@ -650,7 +650,7 @@ bool parser_node_equal (struct parser_node* a, struct parser_node* b)
 }
 
 void
-parser_ast_optimize (struct parser_node* node, std::map<std::string,double>& local_consts)
+parser_ast_optimize (struct parser_node*& node, std::map<std::string,double>& local_consts)
 {
     // No need to free memory because we only call this on ASTs in
     // amrexpr_parser that are allocated from the memory pool.

Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,8 @@ Parser::define (std::string const& func_body)`
`30`	`30`	`try {`
`31`	`31`	`amrexpr_parserparse();`
`32`	`32`	`} catch (const std::runtime_error& e) {`
	`33`	`+ amrexpr_parser_delete_buffer(buffer); // delete buffer allocated by bison`
	`34`	`+ amrexpr_parser_delete_ptrs(); // delete ptrs allocated by amrexpr`
`33`	`35`	`throw std::runtime_error(std::string(e.what()) + " in Parser expression \""`
`34`	`36`	`+ m_data->m_expression + "\"");`
`35`	`37`	`}`