decomp: code review findings about reliability issues in AST and deserialization logic

include/patchestry/AST/OperationBuilder.hpp

@@ -8,6 +8,7 @@
 #pragma once
 
 #include <functional>
+#include <unordered_set>
 
 #include <clang/AST/ASTContext.h>
 #include <clang/AST/OperationKinds.h>
@@ -268,6 +269,9 @@ namespace patchestry::ast {
 
         // Cache for intrinsic function declarations
         std::unordered_map< std::string, clang::FunctionDecl * > intrinsic_decls;
+
+        // Cycle detection for create_temporary forward-reference resolution
+        std::unordered_set< std::string > resolving_temporaries;
     };
 
 } // namespace patchestry::ast

lib/patchestry/AST/FunctionBuilder.cpp

@@ -110,8 +110,12 @@ namespace patchestry::ast {
                     addr = std::stoull(
                         key.substr(first_colon + 1, second_colon - first_colon - 1), nullptr, 16
                     );
-                } catch (...) {
-                    LOG(WARNING) << "BlockKeyComparator: failed to parse hex address in key: " << key;
+                } catch (const std::invalid_argument &) {
+                    LOG(WARNING) << "BlockKeyComparator: non-numeric hex address in key: " << key;
+                    return {std::numeric_limits< uint64_t >::max(),
+                            std::numeric_limits< int64_t >::max()};
+                } catch (const std::out_of_range &) {
+                    LOG(WARNING) << "BlockKeyComparator: hex address out of range in key: " << key;
                     return {std::numeric_limits< uint64_t >::max(),
                             std::numeric_limits< int64_t >::max()};
                 }
@@ -130,8 +134,11 @@ namespace patchestry::ast {
                 int64_t idx = 0;
                 try {
                     idx = std::stoll(idx_str);
-                } catch (...) {
-                    LOG(WARNING) << "BlockKeyComparator: failed to parse block index in key: " << key;
+                } catch (const std::invalid_argument &) {
+                    LOG(WARNING) << "BlockKeyComparator: non-numeric block index in key: " << key;
+                    return {addr, std::numeric_limits< int64_t >::max()};
+                } catch (const std::out_of_range &) {
+                    LOG(WARNING) << "BlockKeyComparator: block index out of range in key: " << key;
                     return {addr, std::numeric_limits< int64_t >::max()};
                 }
 
@@ -338,8 +345,13 @@ namespace patchestry::ast {
                            << param_op->key;
                 continue;
             }
-            const auto &param_type =
-                type_builder.get().get_serialized_types().at(*param_op->type);
+            auto type_iter = type_builder.get().get_serialized_types().find(*param_op->type);
+            if (type_iter == type_builder.get().get_serialized_types().end()) {
+                LOG(ERROR) << "Parameter type not found in serialized types: "
+                           << *param_op->type << ", key: " << param_op->key;
+                continue;
+            }
+            const auto &param_type = type_iter->second;
             auto location = SourceLocation(ctx.getSourceManager(), param_op->key);
 
             auto *param_decl = clang::ParmVarDecl::Create(
@@ -686,9 +698,15 @@ namespace patchestry::ast {
             }
 
             const auto &operation = block.operations.at(operation_key);
+
+            // Save pending_materialized in case create_operation re-enters
+            // (e.g., create_temporary resolving a forward reference triggers
+            // nested operation building that appends to the same vector).
+            auto saved_pending = std::move(pending_materialized);
+            pending_materialized.clear();
+
             if (auto [stmt, should_merge_to_next] = create_operation(ctx, operation); stmt) {
-                // Drain any VarDecl materializations queued during create_operation
-                // (e.g., from create_temporary promoting a cached expr into a VarDecl).
+                // Drain any VarDecl materializations queued during create_operation.
                 // These must appear before the consuming statement in the output.
                 for (auto *pending : pending_materialized) {
                     stmt_vec.push_back(pending);
@@ -699,6 +717,13 @@ namespace patchestry::ast {
                 if (!should_merge_to_next) {
                     stmt_vec.push_back(stmt);
                 }
+            } else {
+                pending_materialized.clear();
+            }
+
+            // Restore any outer-level pending materializations
+            for (auto *s : saved_pending) {
+                pending_materialized.push_back(s);
             }
         }
 

lib/patchestry/AST/OperationBuilder.cpp

@@ -92,7 +92,7 @@ namespace patchestry::ast {
 
     clang::Stmt *OpBuilder::create_parameter(clang::ASTContext &ctx, const Varnode &vnode) {
         if (!vnode.operation || vnode.kind != Varnode::VARNODE_PARAM) {
-            assert(false && "Invalid parameter varnode");
+            LOG(ERROR) << "Invalid parameter varnode\n";
             return nullptr;
         }
 
@@ -109,7 +109,7 @@ namespace patchestry::ast {
 
     clang::Stmt *OpBuilder::create_global(clang::ASTContext &ctx, const Varnode &vnode) {
         if (!vnode.global || vnode.kind != Varnode::VARNODE_GLOBAL) {
-            assert(false && "Invalid global varnode");
+            LOG(ERROR) << "Invalid global varnode\n";
             return {};
         }
 
@@ -150,18 +150,29 @@ namespace patchestry::ast {
         // via a recursive call (which will fall into Case 2).  This prevents re-execution
         // if create_temporary is called again for the same key before create_basic_block
         // reaches the defining block.
-        if (auto maybe_operation = operationFromKey(function, vnode.operation.value())) {
+        const auto &op_key = *vnode.operation;
+        if (resolving_temporaries.contains(op_key)) {
+            LOG(ERROR) << "Cyclic forward reference detected for temporary: " << op_key << "\n";
+            return {};
+        }
+        resolving_temporaries.insert(op_key);
+
+        clang::Stmt *result = nullptr;
+        if (auto maybe_operation = operationFromKey(function, op_key)) {
             auto [stmt, _] = function_builder().create_operation(ctx, *maybe_operation);
             if (stmt) {
-                function_builder().operation_stmts.emplace(*vnode.operation, stmt);
+                function_builder().operation_stmts.emplace(op_key, stmt);
                 // Recurse: will hit Case 1 (if already local) or Case 2.
-                return create_temporary(ctx, function, vnode);
+                result = create_temporary(ctx, function, vnode);
+            } else {
+                result = stmt;
             }
-            return stmt;
+        } else {
+            LOG(ERROR) << "Failed to get operation for key: " << op_key << "\n";
         }
 
-        assert(false && "Failed to get operation for key");
-        return {};
+        resolving_temporaries.erase(op_key);
+        return result;
     }
 
     clang::Stmt *OpBuilder::create_function(clang::ASTContext &ctx, const Varnode &vnode) {

lib/patchestry/AST/OperationStmt.cpp

@@ -307,7 +307,10 @@ namespace patchestry::ast {
         }
 
         auto result = sema().ImpCastExprToType(expr, to_type, kind);
-        assert(!result.isInvalid() && "Failed to make implicit cast expr");
+        if (result.isInvalid()) {
+            LOG(ERROR) << "Failed to make implicit cast expr\n";
+            return nullptr;
+        }
         return result.getAs< clang::Expr >();
     }
 
@@ -723,6 +726,11 @@ namespace patchestry::ast {
         auto loc = SourceLocation(ctx.getSourceManager(), op.key);
         auto *condition_expr =
             clang::dyn_cast< clang::Expr >(create_varnode(ctx, function, *op.condition));
+        if (!condition_expr) {
+            LOG(ERROR) << "Failed to create condition expression for cbranch. key: " << op.key
+                       << "\n";
+            return {};
+        }
 
         clang::Stmt *taken_stmt     = nullptr;
         clang::Stmt *not_taken_stmt = nullptr;
@@ -1260,6 +1268,10 @@ namespace patchestry::ast {
             }
 
             auto operation = operationFromKey(function, *op.target->operation);
+            if (!operation) {
+                LOG(ERROR) << "CALL target operation not found. key: " << op.key << "\n";
+                return {};
+            }
             auto [stmt, _] = function_builder().create_operation(ctx, *operation);
             auto result    = sema().BuildCallExpr(
                 nullptr, clang::dyn_cast< clang::Expr >(stmt), op_loc, arguments, op_loc
@@ -1550,6 +1562,10 @@ namespace patchestry::ast {
         }
         auto location = SourceLocation(ctx.getSourceManager(), op.key);
 
+        if (op.inputs[1].size > UINT32_MAX / 8U) {
+            LOG(ERROR) << "PIECE input size too large, would overflow. key: " << op.key;
+            return {};
+        }
         unsigned low_width = op.inputs[1].size * 8;
         auto merge_to_next = !op.output.has_value();
 
@@ -1638,6 +1654,11 @@ namespace patchestry::ast {
         auto *expr =
             clang::dyn_cast< clang::Expr >(create_varnode(ctx, function, op.inputs[0]));
 
+        if (!expr || !shift_value) {
+            LOG(ERROR) << "Failed to create SUBPIECE input expression. key: " << op.key;
+            return {};
+        }
+
         if (!ctx.hasSameUnqualifiedType(expr->getType(), op_type)) {
             if (auto *casted_expr = make_cast(ctx, expr, op_type, op_location)) {
                 expr = casted_expr;
@@ -2352,12 +2373,12 @@ namespace patchestry::ast {
             for (auto *field : decl->fields()) {
                 auto offset =
                     static_cast< unsigned int >(layout.getFieldOffset(field->getFieldIndex()));
-                if (offset >= target_offset * 8U) {
+                if (offset >= static_cast< uint64_t >(target_offset) * 8U) {
                     return field;
                 }
             }
 
-            assert(false && "Failed to find field decl at offset, check!");
+            LOG(ERROR) << "Failed to find field decl at offset " << target_offset << "\n";
             return nullptr;
         };
 

lib/patchestry/Dialect/Pcode/CMakeLists.txt

@@ -7,7 +7,6 @@ add_mlir_dialect_library(MLIRPcode
   PcodeDialect.cpp
   PcodeOps.cpp
   PcodeTypes.cpp
-  Deserialize.cpp
 
   ADDITIONAL_HEADER_DIRS
     ${PROJECT_SOURCE_DIR}/include/patchestry

lib/patchestry/Ghidra/CMakeLists.txt

@@ -4,7 +4,6 @@
 # LICENSE file found in the root directory of this source tree.
 
 add_library(patchestry_ghidra STATIC
-  PcodeTranslation.cpp
   JsonDeserialize.cpp
 )