From 425cc6794c82e5694f68c3fc5873816253cbba22 Mon Sep 17 00:00:00 2001 From: water111 <48171810+water111@users.noreply.github.com> Date: Wed, 3 Feb 2021 11:07:47 -0500 Subject: [PATCH] [Compiler] Bug fixes (#230) * fix method name and use-after-free during compile error * doc fix --- doc/changelog.md | 5 ++++- doc/goal_doc.md | 19 +++++++++++------- goal_src/goal-lib.gc | 4 ++-- goal_src/kernel/gkernel.gc | 8 ++++---- goalc/compiler/Compiler.h | 3 ++- goalc/compiler/Env.h | 8 ++++++++ goalc/compiler/compilation/Atoms.cpp | 3 ++- goalc/compiler/compilation/Function.cpp | 6 +++++- goalc/compiler/compilation/Type.cpp | 20 ++++++++++++++++--- .../control-statements/methods.static.gc | 4 ++-- .../with_game/test-access-inline-array.gc | 2 +- .../with_game/test-integer-boxed-array.gc | 2 +- test/goalc/test_game_no_debug.cpp | 11 +++++----- test/goalc/test_with_game.cpp | 2 +- 14 files changed, 66 insertions(+), 31 deletions(-) diff --git a/doc/changelog.md b/doc/changelog.md index 70ee70e46a..be432889c0 100644 --- a/doc/changelog.md +++ b/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. \ No newline at end of file +- 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. diff --git a/doc/goal_doc.md b/doc/goal_doc.md index dcb0048583..9f20653199 100644 --- a/doc/goal_doc.md +++ b/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 diff --git a/goal_src/goal-lib.gc b/goal_src/goal-lib.gc index a8fc41914b..7d2635bf85 100644 --- a/goal_src/goal-lib.gc +++ b/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)) ) ) diff --git a/goal_src/kernel/gkernel.gc b/goal_src/kernel/gkernel.gc index 0d35e29323..409eb98740 100644 --- a/goal_src/kernel/gkernel.gc +++ b/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) diff --git a/goalc/compiler/Compiler.h b/goalc/compiler/Compiler.h index 19ecccb8d2..93c0968eac 100644 --- a/goalc/compiler/Compiler.h +++ b/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); diff --git a/goalc/compiler/Env.h b/goalc/compiler/Env.h index f64bd7a0c9..14117b69ff 100644 --- a/goalc/compiler/Env.h +++ b/goalc/compiler/Env.h @@ -108,12 +108,20 @@ class FileEnv : public Env { bool is_empty(); ~FileEnv() = default; + template + T* alloc_val(Args&&... args) { + std::unique_ptr new_obj = std::make_unique(std::forward(args)...); + m_vals.push_back(std::move(new_obj)); + return (T*)m_vals.back().get(); + } + protected: std::string m_name; std::vector> m_functions; std::vector> m_statics; std::unique_ptr m_no_emit_env = nullptr; int m_anon_func_counter = 0; + std::vector> m_vals; // statics FunctionEnv* m_top_level_func = nullptr; diff --git a/goalc/compiler/compilation/Atoms.cpp b/goalc/compiler/compilation/Atoms.cpp index ca83bce137..f807cb5d11 100644 --- a/goalc/compiler/compilation/Atoms.cpp +++ b/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}, diff --git a/goalc/compiler/compilation/Function.cpp b/goalc/compiler/compilation/Function.cpp index 68537fc8ae..9e6ba3ca4b 100644 --- a/goalc/compiler/compilation/Function.cpp +++ b/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(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(get_none()->type()); auto& lambda = place->lambda; auto lambda_ts = m_ts.make_typespec("function"); diff --git a/goalc/compiler/compilation/Type.cpp b/goalc/compiler/compilation/Type.cpp index 866ca794d0..b85f34d310 100644 --- a/goalc/compiler/compilation/Type.cpp +++ b/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); } diff --git a/test/goalc/source_templates/control-statements/methods.static.gc b/test/goalc/source_templates/control-statements/methods.static.gc index 33518fd31d..45c0f1e6f1 100644 --- a/test/goalc/source_templates/control-statements/methods.static.gc +++ b/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 \ No newline at end of file diff --git a/test/goalc/source_templates/with_game/test-access-inline-array.gc b/test/goalc/source_templates/with_game/test-access-inline-array.gc index 31f5d833a0..092068c973 100644 --- a/test/goalc/source_templates/with_game/test-access-inline-array.gc +++ b/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) diff --git a/test/goalc/source_templates/with_game/test-integer-boxed-array.gc b/test/goalc/source_templates/with_game/test-integer-boxed-array.gc index 1d3498ecc0..09d43a6a36 100644 --- a/test/goalc/source_templates/with_game/test-integer-boxed-array.gc +++ b/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... diff --git a/test/goalc/test_game_no_debug.cpp b/test/goalc/test_game_no_debug.cpp index 331f4d4c8a..9c9c4d8aca 100644 --- a/test/goalc/test_game_no_debug.cpp +++ b/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{"#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{"#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{"#t\n0\n"}); // debug segment flag should be disabled. EXPECT_TRUE(compiler.run_test_from_string("(print *debug-segment*) 0") == diff --git a/test/goalc/test_with_game.cpp b/test/goalc/test_with_game.cpp index 621652cd36..f2665a1fe8 100644 --- a/test/goalc/test_with_game.cpp +++ b/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) {