[Compiler] Bug fixes (#230)

* fix method name and use-after-free during compile error

* doc fix

doc/changelog.md

@@ -107,4 +107,7 @@
 - Accessing a constant field of an array now constant propagates the memory offset like field access and avoids a runtime multiply.
 - Fixed a bug where loading or storing a `vf` register from a memory location + constant offset would cause the compiler to throw an error.
 - Accessing array elements uses more efficient indexing for power-of-two element sizes.
-- Added a `local-vars` form for declaring a bunch of local variables for the decompiler.
+- Added a `local-vars` form for declaring a bunch of local variables for the decompiler.
+- Split `method` into `method-of-type` and `method-of-object` to avoid ambiguity
+- Fixed bug where `(-> obj type)` caused a compiler error when `obj` had compile time type of `array` (the fancy boxed array)
+- Fixed use-after-free if the top-level form fails to compile and you continue trying to compile stuff.

doc/goal_doc.md

@@ -1053,12 +1053,19 @@ Bitwise Not
 ## `deftype`
 
 
-## `method`
-Get a method of a type or an object.
-__Warning - I will probably change this in the future.__
+## `method-of-object`
+Get a method of an object.
+
 ```
-(method type method-name)
-(method object method-name)
+(method-of-object object method-name)
+```
+
+This form takes an object and gets the method from it. If the object has runtime type information, will consult the method table at runtime to get a possibly more specific method than what is available at compile time. This uses the same lookup logic as method calling - see the section on method calls for more information.
+
+## `method-of-type`
+Get a method of a type or an object.
+```
+(method-of-type type method-name)
 ```
 
 The first form of this takes a type name and method name and returns a GOAL `function` for this method. For example:
@@ -1067,8 +1074,6 @@ The first form of this takes a type name and method name and returns a GOAL `fun
 ```
 will return the `inspect` method of `string`.
 
-The second form of this takes an object and gets the method from it. If the object has runtime type information, will consult the method table to get a possibly more specific method than what is available at compile time. This uses the same lookup logic as method calling - see the section on method calls for more information.
-
 ## `car` and `cdr`
 Get element from pair
 ```lisp

goal_src/goal-lib.gc

@@ -461,8 +461,8 @@
 
 (defmacro object-new (&rest sz)
   (if (null? sz)
-    `(the ,(current-method-type) ((method object new) allocation type-to-make (the int (-> type-to-make size))))
-    `(the ,(current-method-type) ((method object new) allocation type-to-make ,@sz))
+    `(the ,(current-method-type) ((method-of-type object new) allocation type-to-make (the int (-> type-to-make size))))
+    `(the ,(current-method-type) ((method-of-type object new) allocation type-to-make ,@sz))
     )
   )
 

goal_src/kernel/gkernel.gc

@@ -222,8 +222,8 @@
     
     ;; set up our suspend/resume hooks. By default just use the thread's methods.
     ;; but something else could install a different hook if needed.
-    (set! (-> obj suspend-hook) (method obj thread-suspend))
-    (set! (-> obj resume-hook) (method obj thread-resume))
+    (set! (-> obj suspend-hook) (method-of-object obj thread-suspend))
+    (set! (-> obj resume-hook) (method-of-object obj thread-resume))
     
     ;; remember how much space we have for the backup stack.
     (set! (-> obj stack-size) stack-size)
@@ -848,7 +848,7 @@
     (dotimes (i count)
       ;; create each process
       (let ((old-bro (-> obj child))
-            (next ((method process new) allocation process 'dead stack-size)))
+            (next ((method-of-type process new) allocation process 'dead stack-size)))
         (set! (-> obj child) (as-ppointer next))
         (set! (-> next parent) (as-ppointer obj))
         (set! (-> next pool) obj)
@@ -1145,7 +1145,7 @@
          ;; get the gap
          (set! proc (the process (gap-location obj insert)))
          ;; and allocate! The method new does the offset for us.
-         (set! proc ((method process new) (the symbol proc) process 'process stack-size))
+         (set! proc ((method-of-type process new) (the symbol proc) process 'process stack-size))
          
          ;; update our rec to contain this process.
          (set! (-> rec process) proc)

goalc/compiler/Compiler.h

@@ -385,7 +385,8 @@ class Compiler {
   Val* compile_new(const goos::Object& form, const goos::Object& rest, Env* env);
   Val* compile_car(const goos::Object& form, const goos::Object& rest, Env* env);
   Val* compile_cdr(const goos::Object& form, const goos::Object& rest, Env* env);
-  Val* compile_method(const goos::Object& form, const goos::Object& rest, Env* env);
+  Val* compile_method_of_type(const goos::Object& form, const goos::Object& rest, Env* env);
+  Val* compile_method_of_object(const goos::Object& form, const goos::Object& rest, Env* env);
   Val* compile_addr_of(const goos::Object& form, const goos::Object& rest, Env* env);
   Val* compile_declare_type(const goos::Object& form, const goos::Object& rest, Env* env);
   Val* compile_none(const goos::Object& form, const goos::Object& rest, Env* env);

goalc/compiler/Env.h

@@ -108,12 +108,20 @@ class FileEnv : public Env {
   bool is_empty();
   ~FileEnv() = default;
 
+  template <typename T, class... Args>
+  T* alloc_val(Args&&... args) {
+    std::unique_ptr<T> new_obj = std::make_unique<T>(std::forward<Args>(args)...);
+    m_vals.push_back(std::move(new_obj));
+    return (T*)m_vals.back().get();
+  }
+
  protected:
   std::string m_name;
   std::vector<std::unique_ptr<FunctionEnv>> m_functions;
   std::vector<std::unique_ptr<StaticObject>> m_statics;
   std::unique_ptr<NoEmitEnv> m_no_emit_env = nullptr;
   int m_anon_func_counter = 0;
+  std::vector<std::unique_ptr<Val>> m_vals;
 
   // statics
   FunctionEnv* m_top_level_func = nullptr;

goalc/compiler/compilation/Atoms.cpp

@@ -86,7 +86,8 @@ static const std::unordered_map<
         {"new", &Compiler::compile_new},
         {"car", &Compiler::compile_car},
         {"cdr", &Compiler::compile_cdr},
-        {"method", &Compiler::compile_method},
+        {"method-of-type", &Compiler::compile_method_of_type},
+        {"method-of-object", &Compiler::compile_method_of_object},
         {"declare-type", &Compiler::compile_declare_type},
         {"none", &Compiler::compile_none},
 

goalc/compiler/compilation/Function.cpp

@@ -115,7 +115,11 @@ Val* Compiler::compile_lambda(const goos::Object& form, const goos::Object& rest
     throw_compiler_error(form, "Invalid lambda form");
   }
 
-  auto place = fe->alloc_val<LambdaVal>(get_none()->type());
+  // allocate this lambda from the object file environment. This makes it safe for this to hold
+  // on to references to this as an inlineable function even if the enclosing function fails.
+  // for example, the top-level may (define some-func (lambda...)) and even if top-level fails,
+  // we keep around a reference to some-func to be possibly inlined.
+  auto place = obj_env->alloc_val<LambdaVal>(get_none()->type());
   auto& lambda = place->lambda;
   auto lambda_ts = m_ts.make_typespec("function");
 

goalc/compiler/compilation/Type.cpp

@@ -479,7 +479,7 @@ Val* Compiler::compile_deref(const goos::Object& form, const goos::Object& _rest
           // array-indexable and a structure, we treat it like a structure only if the
           // deref thing is one of the field names. Otherwise, array.
           if (field_name == "content-type" || field_name == "length" ||
-              field_name == "allocated-length") {
+              field_name == "allocated-length" || field_name == "type") {
             result = get_field_of_structure(struct_type, result, field_name, env);
             continue;
           }
@@ -904,8 +904,9 @@ Val* Compiler::compile_cdr(const goos::Object& form, const goos::Object& rest, E
   return result;
 }
 
-// todo, consider splitting into method-of-object and method-of-type?
-Val* Compiler::compile_method(const goos::Object& form, const goos::Object& rest, Env* env) {
+Val* Compiler::compile_method_of_type(const goos::Object& form,
+                                      const goos::Object& rest,
+                                      Env* env) {
   auto args = get_va(form, rest);
   va_check(form, args, {{}, {goos::ObjectType::SYMBOL}}, {});
 
@@ -922,6 +923,19 @@ Val* Compiler::compile_method(const goos::Object& form, const goos::Object& rest
     }
   }
 
+  throw_compiler_error(form, "Cannot get method of type {}: the type is invalid", arg.print());
+  return get_none();
+}
+
+Val* Compiler::compile_method_of_object(const goos::Object& form,
+                                        const goos::Object& rest,
+                                        Env* env) {
+  auto args = get_va(form, rest);
+  va_check(form, args, {{}, {goos::ObjectType::SYMBOL}}, {});
+
+  auto arg = args.unnamed.at(0);
+  auto method_name = symbol_string(args.unnamed.at(1));
+
   auto obj = compile_error_guard(arg, env)->to_gpr(env);
   return compile_get_method_of_object(form, obj, method_name, env);
 }

test/goalc/source_templates/control-statements/methods.static.gc

@@ -2,7 +2,7 @@
 
 ; no longer use process as a test type here because it's no longer built-in so is
 ; only forward declared at this point.
-(format #t "~A~A~%" (eq? (-> type method-table 2) (method type print))
-                    (eq? (-> string method-table 3) (method "test" inspect))
+(format #t "~A~A~%" (eq? (-> type method-table 2) (method-of-type type print))
+                    (eq? (-> string method-table 3) (method-of-object "test" inspect))
                     )
 0

test/goalc/source_templates/with_game/test-access-inline-array.gc

@@ -1,6 +1,6 @@
 (define format _format)
 
-(let* ((print-method (method bfloat print))
+(let* ((print-method (method-of-type bfloat print))
        (my-float (new 'global 'bfloat))
        )
   (set! (-> my-float data) 1.23456)

test/goalc/source_templates/with_game/test-integer-boxed-array.gc

@@ -5,7 +5,7 @@
   (dotimes (i 12)
     (format #t "~D ~D  " i (-> arr i))
     )
-  (format #t "~D ~D ~D~%" (length arr) (asize-of arr) (&- (&-> arr 4) (&-> arr 1)))
+  (format #t "~D ~D ~D ~A~%" (length arr) (asize-of arr) (&- (&-> arr 4) (&-> arr 1)) (-> arr type))
   )
 
 ;; asize should be...

test/goalc/test_game_no_debug.cpp

@@ -13,15 +13,14 @@ TEST(GameNoDebugSegment, Init) {
   compiler.run_test_from_string("(inspect *kernel-context*)");
 
   // these should be equal, both the fallback inspect method
-  EXPECT_TRUE(compiler.run_test_from_string(
-                  "(print (eq? (method kernel-context inspect) (method cpu-thread inspect))) 0") ==
+  EXPECT_TRUE(compiler.run_test_from_string("(print (eq? (method-of-type kernel-context inspect) "
+                                            "(method-of-type cpu-thread inspect))) 0") ==
               std::vector<std::string>{"#t\n0\n"});
 
   // should be below the debug heap.
-  EXPECT_TRUE(
-      compiler.run_test_from_string(
-          "(print (< (the uint (method kernel-context inspect)) (the uint (-> debug base)))) 0") ==
-      std::vector<std::string>{"#t\n0\n"});
+  EXPECT_TRUE(compiler.run_test_from_string("(print (< (the uint (method-of-type kernel-context "
+                                            "inspect)) (the uint (-> debug base)))) 0") ==
+              std::vector<std::string>{"#t\n0\n"});
 
   // debug segment flag should be disabled.
   EXPECT_TRUE(compiler.run_test_from_string("(print *debug-segment*) 0") ==

test/goalc/test_with_game.cpp

@@ -337,7 +337,7 @@ TEST_F(WithGameTests, FancyStatic) {
 TEST_F(WithGameTests, IntegerBoxedArray) {
   runner.run_static_test(
       env, testCategory, "test-integer-boxed-array.gc",
-      {"0 0  1 2  2 4  3 6  4 8  5 10  6 12  7 14  8 16  9 18  10 20  11 22  12 40 6\n0\n"});
+      {"0 0  1 2  2 4  3 6  4 8  5 10  6 12  7 14  8 16  9 18  10 20  11 22  12 40 6 array\n0\n"});
 }
 
 TEST_F(WithGameTests, StaticBoxedArray) {