[decomp] handle handle->process inside an and (#851)

* fix the loader example

* improve compiler error message

* fix missing cast issue

decompiler/IR2/ExpressionHelpers.cpp

@@ -2,6 +2,8 @@
 #include "decompiler/IR2/Form.h"
 #include "decompiler/IR2/Env.h"
 #include "common/goal_constants.h"
+#include "decompiler/IR2/GenericElementMatcher.h"
+#include "decompiler/IR2/FormStack.h"
 
 namespace decompiler {
 
@@ -200,4 +202,154 @@ FormElement* handle_get_property_value(const std::vector<Form*>& forms,
                                                  property_name, default_value, tag_pointer, time,
                                                  TypeSpec("uint128"));
 }
+
+namespace {
+Form* var_to_form(const RegisterAccess& var, FormPool& pool) {
+  return pool.alloc_single_element_form<SimpleAtomElement>(nullptr, SimpleAtom::make_var(var));
+}
+
+}  // namespace
+
+/*!
+ * Recognize the handle->process macro.
+ * If it occurs inside of another and, the part_of_longer_sc argument should be set.
+ */
+FormElement* last_two_in_and_to_handle_get_proc(Form* first,
+                                                Form* second,
+                                                const Env& env,
+                                                FormPool& pool,
+                                                FormStack& stack,
+                                                bool part_of_longer_sc) {
+  constexpr int reg_input_1 = 0;
+  constexpr int reg_input_2 = 1;
+  constexpr int reg_input_3 = 2;
+  constexpr int reg_temp_1 = 10;
+  constexpr int reg_temp_2 = 11;
+  constexpr int reg_temp_3 = 12;
+
+  // only used if part of a longer sc.
+  Form* longer_sc_src = nullptr;  // the source (can be found without repopping)
+  Form longer_sc_first_form;      // the equivalent of the first form for a normal handle->proc
+  RegisterAccess longer_sc_var;   // the second temp var that only appears in this case.
+
+  // check first.
+  Matcher first_matcher =
+      Matcher::op(GenericOpMatcher::condition(IR2_Condition::Kind::NONZERO),
+                  {Matcher::op(GenericOpMatcher::fixed(FixedOperatorKind::L32_NOT_FALSE_CBOOL),
+                               {Matcher::any_reg(reg_input_1)})});
+
+  // if we're part of a longer, the first will actually be (being (set! foo <>) (... foo))
+  // so let's strip out the set, then remember what was being set. If it all works out,
+  // we can just substitute the <> into the macro
+  if (part_of_longer_sc) {
+    if (first->size() != 2) {
+      return nullptr;
+    }
+
+    auto as_var_set = dynamic_cast<SetVarElement*>(first->elts().at(0));
+    if (!as_var_set) {
+      return nullptr;
+    }
+
+    longer_sc_var = as_var_set->dst();
+    longer_sc_src = as_var_set->src();
+
+    longer_sc_first_form.elts().push_back(first->elts().at(1));
+    first = &longer_sc_first_form;
+  }
+
+  auto first_result = match(first_matcher, first);
+  if (!first_result.matched) {
+    return nullptr;
+  }
+
+  // auto first_use_of_in = *first_result.maps.regs.at(reg_input_1);
+  // fmt::print("reg1: {}\n", first_use_of_in.to_string(env));
+
+  auto setup_matcher = Matcher::set_var(
+      Matcher::deref(Matcher::any_reg(reg_input_2), false,
+                     {DerefTokenMatcher::string("process"), DerefTokenMatcher::integer(0)}),
+      reg_temp_1);
+
+  auto if_matcher = Matcher::if_no_else(
+      Matcher::op(
+          GenericOpMatcher::fixed(FixedOperatorKind::EQ),
+          {Matcher::deref(Matcher::any_reg(reg_input_3), false, {DerefTokenMatcher::string("pid")}),
+           Matcher::deref(Matcher::any_reg(reg_temp_2), false,
+                          {DerefTokenMatcher::string("pid")})}),
+      Matcher::any_reg(reg_temp_3));
+
+  auto second_matcher = Matcher::begin({setup_matcher, if_matcher});
+
+  auto second_result = match(second_matcher, second);
+  if (!second_result.matched) {
+    return nullptr;
+  }
+
+  auto in1 = *first_result.maps.regs.at(reg_input_1);
+  auto in2 = *second_result.maps.regs.at(reg_input_2);
+  auto in3 = *second_result.maps.regs.at(reg_input_3);
+
+  auto in_name = in1.to_string(env);
+  if (in_name != in2.to_string(env)) {
+    return nullptr;
+  }
+
+  if (in_name != in3.to_string(env)) {
+    return nullptr;
+  }
+
+  auto temp_name = second_result.maps.regs.at(reg_temp_1)->to_string(env);
+  if (temp_name != second_result.maps.regs.at(reg_temp_2)->to_string(env)) {
+    return nullptr;
+  }
+
+  if (temp_name != second_result.maps.regs.at(reg_temp_3)->to_string(env)) {
+    return nullptr;
+  }
+
+  const auto& temp_use_def = env.get_use_def_info(*second_result.maps.regs.at(reg_temp_1));
+  if (temp_use_def.use_count() != 2 || temp_use_def.def_count() != 1) {
+    fmt::print("failed usedef: {} {}\n", temp_use_def.use_count(), temp_use_def.def_count());
+    return nullptr;
+  }
+
+  if (part_of_longer_sc) {
+    // check that our temporary name matches (it's the var used inside the macro)
+    if (in_name != longer_sc_var.to_string(env)) {
+      fmt::print("failed var name: {} vs {}\n", temp_name, longer_sc_var.to_string(env));
+      return nullptr;
+    }
+
+    // check that our temporary has the right usage pattern.
+    const auto& outer_temp_usedef = env.get_use_def_info(longer_sc_var);
+    if (outer_temp_usedef.use_count() != 3 || outer_temp_usedef.def_count() != 1) {
+      fmt::print("failed usedef2: {} {}\n", outer_temp_usedef.use_count(),
+                 outer_temp_usedef.def_count());
+      return nullptr;
+    }
+
+    return pool.alloc_element<GenericElement>(
+        GenericOperator::make_function(
+            pool.alloc_single_element_form<ConstantTokenElement>(nullptr, "handle->process")),
+        longer_sc_src);
+  } else {
+    // modify use def:
+    auto* menv = const_cast<Env*>(&env);
+    menv->disable_use(in2);
+    menv->disable_use(in3);
+
+    auto repopped = stack.pop_reg(in1, {}, env, true);
+    // fmt::print("repopped: {}\n", repopped->to_string(env));
+
+    if (!repopped) {
+      repopped = var_to_form(in1, pool);
+    }
+
+    return pool.alloc_element<GenericElement>(
+        GenericOperator::make_function(
+            pool.alloc_single_element_form<ConstantTokenElement>(nullptr, "handle->process")),
+        repopped);
+  }
+}
 }  // namespace decompiler

decompiler/IR2/ExpressionHelpers.h

@@ -7,6 +7,7 @@ class FormElement;
 class Form;
 class Env;
 class FormPool;
+class FormStack;
 
 FormElement* handle_get_property_value_float(const std::vector<Form*>& forms,
                                              FormPool& pool,
@@ -21,4 +22,11 @@ FormElement* handle_get_property_struct(const std::vector<Form*>& forms,
 FormElement* handle_get_property_value(const std::vector<Form*>& forms,
                                        FormPool& pool,
                                        const Env& env);
+
+FormElement* last_two_in_and_to_handle_get_proc(Form* first,
+                                                Form* second,
+                                                const Env& env,
+                                                FormPool& pool,
+                                                FormStack& stack,
+                                                bool part_of_longer_sc);
 }  // namespace decompiler

decompiler/IR2/FormExpressionAnalysis.cpp

@@ -3524,97 +3524,27 @@ FormElement* sc_to_handle_get_proc(ShortCircuitElement* elt,
     return nullptr;
   }
 
-  if (elt->entries.size() != 2) {
+  if (elt->entries.size() < 2) {
     return nullptr;
   }
 
-  // fmt::print("candidate: {}\n", elt->to_string(env));
+  Form* last = elt->entries[elt->entries.size() - 1].condition;
+  Form* second_to_last = elt->entries[elt->entries.size() - 2].condition;
 
-  constexpr int reg_input_1 = 0;
-  constexpr int reg_input_2 = 1;
-  constexpr int reg_input_3 = 2;
-  constexpr int reg_temp_1 = 10;
-  constexpr int reg_temp_2 = 11;
-  constexpr int reg_temp_3 = 12;
-
-  // check first.
-  auto first_matcher =
-      Matcher::op(GenericOpMatcher::condition(IR2_Condition::Kind::NONZERO),
-                  {Matcher::op(GenericOpMatcher::fixed(FixedOperatorKind::L32_NOT_FALSE_CBOOL),
-                               {Matcher::any_reg(reg_input_1)})});
-
-  auto first_result = match(first_matcher, elt->entries.at(0).condition);
-  if (!first_result.matched) {
+  auto result = last_two_in_and_to_handle_get_proc(second_to_last, last, env, pool, stack,
+                                                   elt->entries.size() > 2);
+  if (!result) {
     return nullptr;
   }
 
-  // auto first_use_of_in = *first_result.maps.regs.at(reg_input_1);
-  // fmt::print("reg1: {}\n", first_use_of_in.to_string(env));
-
-  auto setup_matcher = Matcher::set_var(
-      Matcher::deref(Matcher::any_reg(reg_input_2), false,
-                     {DerefTokenMatcher::string("process"), DerefTokenMatcher::integer(0)}),
-      reg_temp_1);
-
-  auto if_matcher = Matcher::if_no_else(
-      Matcher::op(
-          GenericOpMatcher::fixed(FixedOperatorKind::EQ),
-          {Matcher::deref(Matcher::any_reg(reg_input_3), false, {DerefTokenMatcher::string("pid")}),
-           Matcher::deref(Matcher::any_reg(reg_temp_2), false,
-                          {DerefTokenMatcher::string("pid")})}),
-      Matcher::any_reg(reg_temp_3));
-
-  auto second_matcher = Matcher::begin({setup_matcher, if_matcher});
-
-  auto second_result = match(second_matcher, elt->entries.at(1).condition);
-  if (!second_result.matched) {
-    return nullptr;
+  if (elt->entries.size() == 2) {
+    // just replace the whole thing
+    return result;
+  } else {
+    elt->entries.pop_back();
+    elt->entries.back().condition = pool.alloc_single_form(elt, result);
   }
 
-  auto in1 = *first_result.maps.regs.at(reg_input_1);
-  auto in2 = *second_result.maps.regs.at(reg_input_2);
-  auto in3 = *second_result.maps.regs.at(reg_input_3);
-
-  auto in_name = in1.to_string(env);
-  if (in_name != in2.to_string(env)) {
-    return nullptr;
-  }
-
-  if (in_name != in3.to_string(env)) {
-    return nullptr;
-  }
-
-  auto temp_name = second_result.maps.regs.at(reg_temp_1)->to_string(env);
-  if (temp_name != second_result.maps.regs.at(reg_temp_2)->to_string(env)) {
-    return nullptr;
-  }
-
-  if (temp_name != second_result.maps.regs.at(reg_temp_3)->to_string(env)) {
-    return nullptr;
-  }
-
-  const auto& temp_use_def = env.get_use_def_info(*second_result.maps.regs.at(reg_temp_1));
-  if (temp_use_def.use_count() != 2 || temp_use_def.def_count() != 1) {
-    return nullptr;
-  }
-
-  // modify use def:
-  auto* menv = const_cast<Env*>(&env);
-  menv->disable_use(in2);
-  menv->disable_use(in3);
-
-  auto repopped = stack.pop_reg(in1, {}, env, true);
-  // fmt::print("repopped: {}\n", repopped->to_string(env));
-
-  if (!repopped) {
-    repopped = var_to_form(in1, pool);
-  }
-
-  return pool.alloc_element<GenericElement>(
-      GenericOperator::make_function(
-          pool.alloc_single_element_form<ConstantTokenElement>(nullptr, "handle->process")),
-      repopped);
-
   return nullptr;
 }
 

decompiler/IR2/FormStack.cpp

@@ -314,6 +314,7 @@ std::vector<FormElement*> FormStack::rewrite(FormPool& pool, const Env& env) con
 
       // we want to untangle coloring moves here
       auto simplified_source = e.source;
+      auto type = e.set_type;
       auto src_as_var = dynamic_cast<SimpleExpressionElement*>(e.source->try_as_single_element());
       if (src_as_var && src_as_var->expr().is_var() && e.is_compactable) {
         bool keep_going = true;
@@ -331,14 +332,16 @@ std::vector<FormElement*> FormStack::rewrite(FormPool& pool, const Env& env) con
               var_to_get = as_one->expr().var();
             }
             simplified_source = last_op_as_set->src();
+            // because we are eliminating, we need to use the source's cast.
+            // to make the code cleaner, we drop casts that would occur in the middle
+            type = last_op_as_set->src_type();
             // result = last_op_as_set->src()->elts();
           }
         }
       }
 
-      auto type = e.set_type;
       auto expected_type = env.get_variable_type(*e.destination, true);
-      if (!env.dts->ts.tc(expected_type, e.set_type)) {
+      if (!env.dts->ts.tc(expected_type, type)) {
         // we would cast. let's see if we can simplify the source to avoid this.
         auto casted = try_cast_simplify(simplified_source, expected_type, pool, env);
         if (casted) {

docs/markdown/progress-notes/changelog.md

@@ -202,4 +202,5 @@
 - It is now an error to have two arguments with the same name.
 - It is now a warning to redefine a constant.
 - Fix a bug where the size of static boxed arrays was only `length` and not `allocated-length`
-- It is now possible to call a method on a forward declared type. The forward declared type must be a basic.
+- It is now possible to call a method on a forward declared type. The forward declared type must be a basic.
+- Using `->` on a plain `pointer` or `inline-array` now generates an error instead of crashing the compiler

goalc/compiler/compilation/Type.cpp

@@ -742,6 +742,10 @@ Val* Compiler::compile_deref(const goos::Object& form, const goos::Object& _rest
     }
 
     if (result->type().base_type() == "inline-array") {
+      if (!result->type().has_single_arg()) {
+        throw_compiler_error(form, "Cannot dereference an inline-array with type {}",
+                             result->type().print());
+      }
       auto di = m_ts.get_deref_info(result->type());
       auto base_type = di.result_type;
       assert(di.can_deref);
@@ -755,6 +759,10 @@ Val* Compiler::compile_deref(const goos::Object& form, const goos::Object& _rest
         result = fe->alloc_val<MemoryOffsetVal>(di.result_type, result, offset);
       }
     } else if (result->type().base_type() == "pointer") {
+      if (!result->type().has_single_arg()) {
+        throw_compiler_error(form, "Cannot dereference a pointer with type {}",
+                             result->type().print());
+      }
       auto di = m_ts.get_deref_info(result->type());
       auto base_type = di.result_type;
       assert(di.mem_deref);

test/decompiler/reference/engine/load/loader_REF.gc

@@ -397,7 +397,6 @@
 ;; definition for method 10 of type external-art-buffer
 ;; WARN: Found some very strange gotos. Check result carefully, this is not well tested.
 (defmethod update external-art-buffer ((obj external-art-buffer))
-  (local-vars (v1-54 handle))
   (when
    (or
     (not (name= (-> obj pending-load-file) (-> obj load-file)))
@@ -552,16 +551,8 @@
        )
       )
      (('locked)
-      (when (and (not (-> obj locked?)) (begin
-                                         (set! v1-54 (-> obj load-file-owner))
-                                         (nonzero? (l32-false-check v1-54))
-                                         )
-             (let ((a0-46 (-> v1-54 process 0)))
-              (if (= (-> v1-54 pid) (-> a0-46 pid))
-               a0-46
-               )
-              )
-             )
+      (when
+       (and (not (-> obj locked?)) (handle->process (-> obj load-file-owner)))
        (link-file obj (-> obj art-group))
        (set! (-> obj other locked?) #t)
        (set! (-> obj status) 'active)

test/offline/offline_test_main.cpp

@@ -171,8 +171,6 @@ const std::unordered_set<std::string> g_functions_to_skip_compiling = {
     // camera
     "slave-set-rotation!", "v-slrp2!", "v-slrp3!",  // vector-dot involving the stack
 
-    // loader - decompiler bug with detecting handle macros
-    "(method 10 external-art-buffer)",
     // function returning float with a weird cast.
     "debug-menu-item-var-make-float"};