[decompiler] bug fixes related to infinite loop CFGs, argument regist…

…ers (#622)

* temp

* clean up

* more clean

decompiler/Function/CfgVtx.cpp

@@ -912,7 +912,7 @@ bool ControlFlowGraph::find_goto_not_end() {
   return replaced;
 }
 
-bool ControlFlowGraph::is_sequence(CfgVtx* b0, CfgVtx* b1) {
+bool ControlFlowGraph::is_sequence(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops) {
   if (!b0 || !b1)
     return false;
 
@@ -939,40 +939,49 @@ bool ControlFlowGraph::is_sequence(CfgVtx* b0, CfgVtx* b1) {
     return false;
   if (!b1->has_pred(b0))
     return false;
-  if (b1->succ_branch == b0)
+
+  if (!allow_self_loops && b1->succ_branch == b0)
     return false;
+
   return true;
 }
 
-bool ControlFlowGraph::is_sequence_of_non_sequences(CfgVtx* b0, CfgVtx* b1) {
+bool ControlFlowGraph::is_sequence_of_non_sequences(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops) {
   if (!b0 || !b1)
     return false;
   if (dynamic_cast<SequenceVtx*>(b0) || dynamic_cast<SequenceVtx*>(b1))
     return false;
-  return is_sequence(b0, b1);
+
+  return is_sequence(b0, b1, allow_self_loops);
 }
 
-bool ControlFlowGraph::is_sequence_of_sequence_and_non_sequence(CfgVtx* b0, CfgVtx* b1) {
+bool ControlFlowGraph::is_sequence_of_sequence_and_non_sequence(CfgVtx* b0,
+                                                                CfgVtx* b1,
+                                                                bool allow_self_loops) {
   if (!b0 || !b1)
     return false;
   if (!dynamic_cast<SequenceVtx*>(b0))
     return false;
   if (dynamic_cast<SequenceVtx*>(b1))
     return false;
-  return is_sequence(b0, b1);
+  return is_sequence(b0, b1, allow_self_loops);
 }
 
-bool ControlFlowGraph::is_sequence_of_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1) {
+bool ControlFlowGraph::is_sequence_of_sequence_and_sequence(CfgVtx* b0,
+                                                            CfgVtx* b1,
+                                                            bool allow_self_loops) {
   if (!b0 || !b1)
     return false;
   if (!dynamic_cast<SequenceVtx*>(b0))
     return false;
   if (!dynamic_cast<SequenceVtx*>(b1))
     return false;
-  return is_sequence(b0, b1);
+  return is_sequence(b0, b1, allow_self_loops);
 }
 
-bool ControlFlowGraph::is_sequence_of_non_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1) {
+bool ControlFlowGraph::is_sequence_of_non_sequence_and_sequence(CfgVtx* b0,
+                                                                CfgVtx* b1,
+                                                                bool allow_self_loops) {
   if (!b0 || !b1) {
     return false;
   }
@@ -981,21 +990,21 @@ bool ControlFlowGraph::is_sequence_of_non_sequence_and_sequence(CfgVtx* b0, CfgV
     return false;
   if (!dynamic_cast<SequenceVtx*>(b1))
     return false;
-  return is_sequence(b0, b1);
+  return is_sequence(b0, b1, allow_self_loops);
 }
 
 /*!
  * Find and insert at most one sequence. Return true if sequence is inserted.
  * To generate more readable debug output, we should aim to run this as infrequent and as
  * late as possible, to avoid condition vertices with tons of extra junk packed in.
  */
-bool ControlFlowGraph::find_seq_top_level() {
+bool ControlFlowGraph::find_seq_top_level(bool allow_self_loops) {
   bool replaced = false;
   for_each_top_level_vtx([&](CfgVtx* vtx) {
     auto* b0 = vtx;
     auto* b1 = vtx->next;
 
-    if (is_sequence_of_non_sequences(b0, b1)) {  // todo, avoid nesting sequences.
+    if (is_sequence_of_non_sequences(b0, b1, allow_self_loops)) {  // todo, avoid nesting sequences.
       replaced = true;
 
       auto* new_seq = alloc<SequenceVtx>();
@@ -1028,7 +1037,7 @@ bool ControlFlowGraph::find_seq_top_level() {
       return false;
     }
 
-    if (is_sequence_of_sequence_and_non_sequence(b0, b1)) {
+    if (is_sequence_of_sequence_and_non_sequence(b0, b1, allow_self_loops)) {
       //      printf("make seq type 2 %s %s\n", b0->to_string().c_str(), b1->to_string().c_str());
       replaced = true;
       auto* seq = dynamic_cast<SequenceVtx*>(b0);
@@ -1051,7 +1060,7 @@ bool ControlFlowGraph::find_seq_top_level() {
       return false;
     }
 
-    if (is_sequence_of_non_sequence_and_sequence(b0, b1)) {
+    if (is_sequence_of_non_sequence_and_sequence(b0, b1, allow_self_loops)) {
       replaced = true;
       auto* seq = dynamic_cast<SequenceVtx*>(b1);
       assert(seq);
@@ -1070,7 +1079,7 @@ bool ControlFlowGraph::find_seq_top_level() {
       return false;
     }
 
-    if (is_sequence_of_sequence_and_sequence(b0, b1)) {
+    if (is_sequence_of_sequence_and_sequence(b0, b1, allow_self_loops)) {
       //      printf("make seq type 3 %s %s\n", b0->to_string().c_str(), b1->to_string().c_str());
       replaced = true;
       auto* seq = dynamic_cast<SequenceVtx*>(b0);
@@ -2134,7 +2143,7 @@ std::shared_ptr<ControlFlowGraph> build_cfg(const LinkedObjectFile& file,
     changed = changed || cfg->find_cond_w_else(cond_with_else_hack);
 
     changed = changed || cfg->find_while_loop_top_level();
-    changed = changed || cfg->find_seq_top_level();
+    changed = changed || cfg->find_seq_top_level(false);
     changed = changed || cfg->find_short_circuits();
     changed = changed || cfg->find_cond_n_else();
 
@@ -2145,6 +2154,10 @@ std::shared_ptr<ControlFlowGraph> build_cfg(const LinkedObjectFile& file,
       changed = changed || cfg->find_infinite_loop();
     };
 
+    if (!changed) {
+      changed = changed || cfg->find_seq_top_level(true);
+    }
+
     if (!changed) {
       changed = changed || cfg->find_goto_not_end();
     }

decompiler/Function/CfgVtx.h

@@ -322,7 +322,7 @@ class ControlFlowGraph {
   bool find_cond_n_else();
 
   //  bool find_if_else_top_level();
-  bool find_seq_top_level();
+  bool find_seq_top_level(bool allow_self_loops);
   bool find_while_loop_top_level();
   bool find_until_loop();
   bool find_until1_loop();
@@ -363,11 +363,11 @@ class ControlFlowGraph {
  private:
   //  bool compact_one_in_top_level();
   //  bool is_if_else(CfgVtx* b0, CfgVtx* b1, CfgVtx* b2, CfgVtx* b3);
-  bool is_sequence(CfgVtx* b0, CfgVtx* b1);
-  bool is_sequence_of_non_sequences(CfgVtx* b0, CfgVtx* b1);
-  bool is_sequence_of_sequence_and_non_sequence(CfgVtx* b0, CfgVtx* b1);
-  bool is_sequence_of_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1);
-  bool is_sequence_of_non_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1);
+  bool is_sequence(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops);
+  bool is_sequence_of_non_sequences(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops);
+  bool is_sequence_of_sequence_and_non_sequence(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops);
+  bool is_sequence_of_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops);
+  bool is_sequence_of_non_sequence_and_sequence(CfgVtx* b0, CfgVtx* b1, bool allow_self_loops);
   bool is_while_loop(CfgVtx* b0, CfgVtx* b1, CfgVtx* b2);
   bool is_until_loop(CfgVtx* b1, CfgVtx* b2);
   bool is_goto_end_and_unreachable(CfgVtx* b0, CfgVtx* b1);

decompiler/IR2/FormExpressionAnalysis.cpp

@@ -1846,7 +1846,8 @@ void FunctionCallElement::update_from_stack(const Env& env,
     function_type = tp_type.typespec();
   }
 
-  bool swap_function = tp_type.kind == TP_Type::Kind::NON_VIRTUAL_METHOD && true;
+  bool swap_function =
+      tp_type.kind == TP_Type::Kind::NON_VIRTUAL_METHOD && all_pop_vars.size() >= 2;
   if (tp_type.kind == TP_Type::Kind::NON_VIRTUAL_METHOD) {
     // this is a hack to make some weird macro for calling res-lump methods work
     if (env.dts->ts.tc(TypeSpec("res-lump"), tp_type.method_from_type())) {

decompiler/analysis/stack_spill.cpp

@@ -78,6 +78,8 @@ struct StackInstrInfo {
 constexpr StackInstrInfo stack_instrs[] = {{InstructionKind::SQ, false, 16, false},
                                            {InstructionKind::LQ, true, 16, false},
                                            {InstructionKind::SW, false, 4, false},
+                                           {InstructionKind::SB, false, 1, false},
+                                           {InstructionKind::LBU, true, 1, false},
                                            //{InstructionKind::LWU, true, 4, false}
                                            {InstructionKind::SD, false, 8, false},
                                            {InstructionKind::SWC1, false, 4, false},

decompiler/analysis/variable_naming.cpp

@@ -279,6 +279,16 @@ bool is_possible_coloring_move(Register dst, Register src) {
   return false;
 }
 
+namespace {
+int arg_count(const Function& f) {
+  if (f.type.arg_count() > 0) {
+    return f.type.arg_count() - 1;
+  } else {
+    return 0;
+  }
+}
+}  // namespace
+
 /*!
  * Create a "really crude" SSA, as described in
  * "Aycock and Horspool Simple Generation of Static Single-Assignment Form"
@@ -310,6 +320,8 @@ SSA make_rc_ssa(const Function& function, const RegUsageInfo& rui, const Functio
     // local map: current register names at the current op.
     std::unordered_map<Register, VarSSA, Register::hash> current_regs;
 
+    // if we're block zero, write function arguments:
+
     // initialize phis. this is only done on:
     //  - variables live out at the first op
     //  - variables read by the first op
@@ -336,6 +348,19 @@ SSA make_rc_ssa(const Function& function, const RegUsageInfo& rui, const Functio
       }
     }
 
+    if (block_id == 0) {
+      SSA::Ins ins(-1);
+      for (int i = 0; i < arg_count(function); i++) {
+        auto dest_reg = Register::get_arg_reg(i);
+        auto it = current_regs.find(dest_reg);
+        if (it == current_regs.end()) {
+          current_regs.insert(std::make_pair(dest_reg, ssa.get_phi_dest(block_id, dest_reg)));
+        }
+        ins.src.push_back(current_regs.at(dest_reg));
+      }
+      ssa.blocks.at(block_id).ins.push_back(ins);
+    }
+
     // loop over ops, creating and reading from variables as needed.
     for (int op_id = start_op; op_id < end_op; op_id++) {
       const auto& op = ops.ops.at(op_id);
@@ -546,7 +571,7 @@ void SSA::merge_all_phis() {
  * Remaps all SSA variable ids to final variable IDs.
  * This forces you to have all positive, consecutive IDs, with 0 being the entry value.
  */
-void SSA::remap() {
+void SSA::remap(int) {
   // this keeps the order of variable assignments in the instruction order, not var_id order.
   struct VarIdRecord {
     std::unordered_set<int> set;
@@ -676,6 +701,9 @@ void SSA::make_vars(const Function& function, const DecompilerTypeSystem& dts) {
     const TypeState* init_types = &function.ir2.env.get_types_at_block_entry(block_id);
     for (auto& instr : block.ins) {
       auto op_id = instr.op_id;
+      if (op_id < 0) {
+        continue;
+      }
 
       const TypeState* end_types = &function.ir2.env.get_types_after_op(op_id);
 
@@ -762,6 +790,9 @@ VariableNames SSA::get_vars() const {
     const auto& block = blocks.at(block_id);
     for (auto& instr : block.ins) {
       auto op_id = instr.op_id;
+      if (op_id < 0) {
+        continue;
+      }
       if (instr.dst.has_value()) {
         auto& ids = result.write_opid_to_varid[instr.dst->reg()];
         if (int(ids.size()) <= op_id) {
@@ -776,6 +807,9 @@ VariableNames SSA::get_vars() const {
     const auto& block = blocks.at(block_id);
     for (auto& instr : block.ins) {
       auto op_id = instr.op_id;
+      if (op_id < 0) {
+        continue;
+      }
       for (auto& src : instr.src) {
         auto& ids = result.read_opid_to_varid[src.reg()];
         if (int(ids.size()) <= op_id) {
@@ -944,7 +978,7 @@ std::optional<VariableNames> run_variable_renaming(const Function& function,
   // merge same vars (decided this made things worse)
 
   // do rename
-  ssa.remap();
+  ssa.remap(arg_count(function));
   if (debug_prints) {
     fmt::print("{}", ssa.print());
   }

decompiler/analysis/variable_naming.h

@@ -81,9 +81,10 @@ class VarMapSSA {
  private:
   int get_next_var_id(Register reg);
 
+  // var id's are per register.
   struct Entry {
-    int var_id = -1;
-    int entry_id = -1;
+    int var_id = -1;    // our ID as a program variable (used for output)
+    int entry_id = -1;  // our index in the entry list (used for remapping)
     Register reg;
   };
 
@@ -142,7 +143,7 @@ struct SSA {
 
   bool simplify();
   void merge_all_phis();
-  void remap();
+  void remap(int nargs);
   void make_vars(const Function& function, const DecompilerTypeSystem& dts);
   std::unordered_map<RegId, UseDefInfo, RegId::hash> get_use_def_info(
       const RegAccessMap<int>& ssa_info) const;

decompiler/config/all-types.gc

@@ -6174,7 +6174,7 @@
   )
 
 (deftype ocean-mid-masks (basic)
-  ((data uint32  :offset-assert 4)
+  ((data uint32 :offset-assert 4)
    )
   :pack-me
   :method-count-assert 9
@@ -6242,10 +6242,10 @@
    (far-color           vector        :inline :offset-assert 32)
    (ocean-spheres       ocean-spheres         :offset-assert 48)
    (ocean-colors        ocean-colors          :offset-assert 52)
-   (ocean-mid-indices   basic                 :offset-assert 56)
-   (ocean-trans-indices basic                 :offset-assert 60)
-   (ocean-near-indices  basic                 :offset-assert 64)
-   (ocean-mid-masks     basic                 :offset-assert 68)
+   (ocean-mid-indices   basic     :offset-assert 56)
+   (ocean-trans-indices basic   :offset-assert 60)
+   (ocean-near-indices  basic    :offset-assert 64)
+   (ocean-mid-masks     basic       :offset-assert 68)
    )
   :method-count-assert 9
   :size-assert         #x48
@@ -20414,7 +20414,7 @@
 
 ;; - Unknowns
 
-;;(define-extern *ocean-map* object) ;; unknown type
+(define-extern *ocean-map* ocean-map) ;; unknown type
 ;;(define-extern *swamp-low-ocean-marker* object) ;; unknown type
 
 
@@ -20464,15 +20464,15 @@
 (define-extern ocean-mid-add-constants function)
 (define-extern ocean-mid-add-call function)
 (define-extern ocean-mid-add-upload function)
-(define-extern ocean-mid-add-call-flush function)
-(define-extern draw-ocean-transition function)
+(define-extern ocean-mid-add-call-flush (function dma-buffer uint none))
+(define-extern draw-ocean-transition (function dma-buffer none))
 (define-extern draw-ocean-mid-seams function)
 (define-extern ocean-seams-add-constants function)
 (define-extern ocean-mid-add-upload-top function)
 (define-extern ocean-mid-add-upload-bottom function)
 (define-extern ocean-mid-add-upload-middle function)
-(define-extern ocean-mid-camera-masks-bit? function)
-(define-extern ocean-mid-mask-ptrs-bit? function)
+(define-extern ocean-mid-camera-masks-bit? (function uint uint))
+(define-extern ocean-mid-mask-ptrs-bit? (function uint uint))
 (define-extern ocean-mid-add-upload-table function)
 (define-extern ocean-mid-camera-masks-set! function)
 (define-extern ocean-mid-add-matrices function)
@@ -20494,10 +20494,10 @@
 
 ;; - Functions
 
-(define-extern ocean-make-trans-camera-masks function)
-(define-extern ocean-trans-add-upload-strip function)
-(define-extern ocean-trans-add-constants function)
-(define-extern draw-ocean-transition-seams function)
+(define-extern ocean-make-trans-camera-masks (function uint uint uint uint none))
+(define-extern ocean-trans-add-upload-strip (function dma-buffer uint uint uint uint none))
+(define-extern ocean-trans-add-constants (function dma-buffer none))
+(define-extern draw-ocean-transition-seams (function dma-buffer none))
 (define-extern ocean-trans-camera-masks-bit? function)
 (define-extern ocean-trans-add-upload function)
 (define-extern ocean-trans-mask-ptrs-bit? function)

decompiler/config/jak1_ntsc_black_label/stack_structures.jsonc

@@ -376,5 +376,16 @@
   "add-debug-curve": [[16, "vector"], [32, "vector"]],
   "add-debug-points": [[16, "vector"]],
   "add-debug-light": [[16, "vector"]],
-  "dma-timeout-cam": [[16, "vector"], [32, "matrix"]]
+  "dma-timeout-cam": [[16, "vector"], [32, "matrix"]],
+
+  "(method 18 tracking-spline)": [
+    [16, "tracking-spline-sampler"],
+    [32, "tracking-spline-sampler"]
+  ],
+
+  "draw-ocean-transition": [
+    [16, "sphere"]
+  ]
+
+
 }