Fix/fuzzer bugs

CHANGELOG.md

@@ -154,6 +154,8 @@
 - captures are not renamed anymore by the NameResolutionPass (which used to fully qualify captured names when possible, which isn't desirable: when you capture `&foo`, you expect to be able to use `.foo` not `.module:foo`)
 - when loading a module, its mappings are loaded in the current scope instead of the global scope
 - argument order in the CLI changed: the file to run (and its optional script arguments) are now last, to be more consistent with all the other existing tooling (Python, Docker...)
+- VM stack size has been upped to 4096 + 256, to have a buffer to be able to catch stack overflows without hindering performances too much
+- we can not create a variable in a function, shadowing said function, to prevent weird bugs when trying to do recursion for example
 
 ### Removed
 - removed unused `NodeType::Closure`

include/Ark/Compiler/Macros/Processor.hpp

@@ -151,9 +151,10 @@ namespace Ark::internal
          * @param node a list node with a macro application, eg (= a b)
          * @param expected expected argument count, not counting the macro
          * @param name the name of the macro being applied
+         * @param is_expansion if the error message should switch from "Interpreting ..." to "When expanding ..."
          * @param kind the macro kind, empty by default (eg "operator", "condition")
          */
-        void checkMacroArgCountEq(const Node& node, std::size_t expected, const std::string& name, const std::string& kind = "");
+        void checkMacroArgCountEq(const Node& node, std::size_t expected, const std::string& name, bool is_expansion = false, const std::string& kind = "");
 
         /**
          * @brief Check if the given node has at least the provided argument count, otherwise throws an error

include/Ark/Constants.hpp.in

@@ -69,10 +69,12 @@ namespace Ark
 
     constexpr uint16_t MaxValue16Bits = std::numeric_limits<uint16_t>::max();
 
-    constexpr std::size_t MaxNestedNodes = 1024;  ///< Maximum number of nodes that can be nested while parsing code
-    constexpr std::size_t MaxMacroProcessingDepth = 256;  ///< Controls the number of recursive calls to MacroProcessor::processNode
+    constexpr std::size_t MaxNestedNodes = 1024;           ///< Maximum number of nodes that can be nested while parsing code
+    constexpr std::size_t MaxMacroProcessingDepth = 256;   ///< Controls the number of recursive calls to MacroProcessor::processNode
     constexpr std::size_t MaxMacroUnificationDepth = 256;  ///< Controls the number of recursive calls to MacroProcessor::unify
     constexpr std::size_t VMStackSize = 4096;
+    constexpr std::size_t VMOverflowBufferSize = 256;                                          ///< Additional number of elements the stack can tolerate
+    constexpr std::size_t VMStackSizeWithOverflowBuffer = VMStackSize + VMOverflowBufferSize;  ///< Exceeding VMStackSize, even if we are below VMStackSizeWithOverflowBuffer, means we stack overflowed
     constexpr std::size_t ScopeStackSize = 8192;
 }
 

include/Ark/VM/ExecutionContext.hpp

@@ -33,6 +33,7 @@ namespace Ark::internal
         uint16_t fc {};     ///< Frame count
         uint16_t last_symbol;
         const bool primary;  ///< Tells if the current ExecutionContext is the primary one or not
+        uint16_t inst_exec_counter {};
         std::atomic_bool active;
 
         std::optional<ClosureScope> saved_scope {};  ///< Scope created by CAPTURE <x> instructions, used by the MAKE_CLOSURE instruction
@@ -42,7 +43,7 @@ namespace Ark::internal
         std::vector<ScopeView> locals {};
         std::array<ScopeView::pair_t, ScopeStackSize> scopes_storage {};  ///< All the ScopeView use this array to store id->value
 
-        std::array<Value, VMStackSize> stack {};
+        std::array<Value, VMStackSizeWithOverflowBuffer> stack {};
 
         ExecutionContext() noexcept :
             last_symbol(MaxValue16Bits),

include/Ark/VM/VM.hpp

@@ -272,23 +272,23 @@ namespace Ark
          * @param value
          * @param context
          */
-        inline void push(const Value& value, internal::ExecutionContext& context);
+        inline void push(const Value& value, internal::ExecutionContext& context) noexcept;
 
         /**
          * @brief Push a value on the stack
          *
          * @param value
          * @param context
          */
-        inline void push(Value&& value, internal::ExecutionContext& context);
+        inline void push(Value&& value, internal::ExecutionContext& context) noexcept;
 
         /**
          * @brief Push a value on the stack as a reference
          *
          * @param valptr
          * @param context
          */
-        inline void push(Value* valptr, internal::ExecutionContext& context);
+        inline void push(Value* valptr, internal::ExecutionContext& context) noexcept;
 
         /**
          * @brief Pop a value from the stack and resolve it if possible, then return it

include/Ark/VM/VM.inl

@@ -185,19 +185,19 @@ inline Value* VM::peekAndResolveAsPtr(internal::ExecutionContext& context)
     return &m_undefined_value;
 }
 
-inline void VM::push(const Value& value, internal::ExecutionContext& context)
+inline void VM::push(const Value& value, internal::ExecutionContext& context) noexcept
 {
     context.stack[context.sp] = value;
     ++context.sp;
 }
 
-inline void VM::push(Value&& value, internal::ExecutionContext& context)
+inline void VM::push(Value&& value, internal::ExecutionContext& context) noexcept
 {
     context.stack[context.sp] = std::move(value);
     ++context.sp;
 }
 
-inline void VM::push(Value* valptr, internal::ExecutionContext& context)
+inline void VM::push(Value* valptr, internal::ExecutionContext& context) noexcept
 {
     context.stack[context.sp].m_type = ValueType::Reference;
     context.stack[context.sp].m_value = valptr;

src/arkreactor/Compiler/Lowerer/ASTLowerer.cpp

@@ -327,6 +327,11 @@ namespace Ark::internal
 
     void ASTLowerer::compileIf(Node& x, const Page p, const bool is_result_unused, const bool is_terminal)
     {
+        if (x.constList().size() == 1)
+            buildAndThrowError("Invalid condition: missing 'cond' and 'then' nodes, expected (if cond then)", x);
+        if (x.constList().size() == 2)
+            buildAndThrowError(fmt::format("Invalid condition: missing 'then' node, expected (if {} then)", x.constList()[1].repr()), x);
+
         // compile condition
         compileExpression(x.list()[1], p, false, false);
         page(p).back().setSourceLocation(x.constList()[1].filename(), x.constList()[1].position().start.line);
@@ -445,6 +450,9 @@ namespace Ark::internal
         const std::string name = x.constList()[1].string();
         uint16_t i = addSymbol(x.constList()[1]);
 
+        if (!m_opened_vars.empty() && m_opened_vars.top() == name)
+            buildAndThrowError("Can not define a variable using the same name as the function it is defined inside", x);
+
         const bool is_function = x.constList()[2].isFunction();
         if (is_function)
         {
@@ -604,6 +612,9 @@ namespace Ark::internal
             }
             else
             {
+                if (!nodeProducesOutput(node))
+                    buildAndThrowError(fmt::format("Can not call `{}', as it doesn't return a value", node.repr()), node);
+
                 m_temp_pages.emplace_back();
                 const auto proc_page = Page { .index = m_temp_pages.size() - 1u, .is_temp = true };
 
@@ -614,7 +625,7 @@ namespace Ark::internal
                     // We can skip the LOAD_SYMBOL function_name and directly push the current
                     // function page, which will be quicker than a local variable resolution.
                     // We set its argument to the symbol id of the function we are calling,
-                    // so that the VM knowns the name of the last called function.
+                    // so that the VM knows the name of the last called function.
                     page(proc_page).emplace_back(GET_CURRENT_PAGE_ADDR, addSymbol(node));
                 }
                 else

src/arkreactor/Compiler/Macros/Executors/Function.cpp

@@ -38,6 +38,9 @@ namespace Ark::internal
                 if (j >= args_needed)
                     break;
 
+                // todo: this fails because we don't have a string because we are defining a (macro ! (call ...args) ...) inside another (macro ! (call ...args) ...)
+                //       most likely the first macro got applied to another macro. We shouldn't apply macro on macros
+                // assert(args.list()[j].nodeType() == NodeType::String || args.list()[j].nodeType() == NodeType::Spread);  // todo: temp
                 const std::string& arg_name = args.list()[j].string();
                 if (args.list()[j].nodeType() == NodeType::Symbol)
                 {
@@ -111,8 +114,24 @@ namespace Ark::internal
         }
         else if (target.isListLike())
         {
-            for (std::size_t i = 0; i < target.list().size(); ++i)
-                unify(map, target.list()[i], &target, i, unify_depth + 1);
+            if (target.nodeType() == NodeType::Macro && target.list()[0].nodeType() == NodeType::Symbol)
+            {
+                if (const std::string macro_name = target.list()[0].string(); map.contains(macro_name))
+                    throwMacroProcessingError(
+                        fmt::format(
+                            "Can not define a macro by reusing the argument name `{}'",
+                            macro_name),
+                        target);
+
+                // proceed for expansion only on the value of each macro
+                unify(map, target.list().back(), &target, target.list().size() - 1, unify_depth + 1);
+            }
+            else
+            {
+                // proceed for expansion on normal nodes, we can safely run on all subnodes
+                for (std::size_t i = 0; i < target.list().size(); ++i)
+                    unify(map, target.list()[i], &target, i, unify_depth + 1);
+            }
         }
         else if (target.nodeType() == NodeType::Spread)
         {

src/arkreactor/Compiler/Macros/Processor.cpp

@@ -208,19 +208,31 @@ namespace Ark::internal
         return false;
     }
 
-    void MacroProcessor::checkMacroArgCountEq(const Node& node, std::size_t expected, const std::string& name, const std::string& kind)
+    void MacroProcessor::checkMacroArgCountEq(const Node& node, std::size_t expected, const std::string& name, const bool is_expansion, const std::string& kind)
     {
         const std::size_t argcount = node.constList().size();
         if (argcount != expected + 1)
-            throwMacroProcessingError(
-                fmt::format(
-                    "Interpreting a `{}'{} with {} argument{}, expected {}.",
-                    name,
-                    kind.empty() ? kind : " " + kind,
-                    argcount - 1,
-                    argcount > 2 ? "s" : "",
-                    expected),
-                node);
+        {
+            if (is_expansion)
+                throwMacroProcessingError(
+                    fmt::format(
+                        "When expanding `{}' inside a macro, got {} argument{}, expected {}",
+                        name,
+                        argcount - 1,
+                        argcount > 2 ? "s" : "",
+                        expected),
+                    node);
+            else
+                throwMacroProcessingError(
+                    fmt::format(
+                        "Interpreting a `{}'{} with {} argument{}, expected {}.",
+                        name,
+                        kind.empty() ? kind : " " + kind,
+                        argcount - 1,
+                        argcount > 2 ? "s" : "",
+                        expected),
+                    node);
+        }
     }
 
     void MacroProcessor::checkMacroArgCountGe(const Node& node, std::size_t expected, const std::string& name, const std::string& kind)
@@ -260,42 +272,42 @@ namespace Ark::internal
             }
             else if (name == "=" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, "=", "condition");
+                checkMacroArgCountEq(node, 2, "=", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return (one == two) ? getTrueNode() : getFalseNode();
             }
             else if (name == "!=" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, "!=", "condition");
+                checkMacroArgCountEq(node, 2, "!=", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return (one != two) ? getTrueNode() : getFalseNode();
             }
             else if (name == "<" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, "<", "condition");
+                checkMacroArgCountEq(node, 2, "<", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return (one < two) ? getTrueNode() : getFalseNode();
             }
             else if (name == ">" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, ">", "condition");
+                checkMacroArgCountEq(node, 2, ">", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return !(one < two) && (one != two) ? getTrueNode() : getFalseNode();
             }
             else if (name == "<=" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, "<=", "condition");
+                checkMacroArgCountEq(node, 2, "<=", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return one < two || one == two ? getTrueNode() : getFalseNode();
             }
             else if (name == ">=" && is_not_body)
             {
-                checkMacroArgCountEq(node, 2, ">=", "condition");
+                checkMacroArgCountEq(node, 2, ">=", /* is_expansion= */ false, "condition");
                 const Node one = evaluate(node.list()[1], depth + 1, is_not_body);
                 const Node two = evaluate(node.list()[2], depth + 1, is_not_body);
                 return !(one < two) ? getTrueNode() : getFalseNode();
@@ -362,7 +374,7 @@ namespace Ark::internal
             }
             else if (name == "not" && is_not_body)
             {
-                checkMacroArgCountEq(node, 1, "not", "condition");
+                checkMacroArgCountEq(node, 1, "not", /* is_expansion= */ false, "condition");
                 return (!isTruthy(evaluate(node.list()[1], depth + 1, is_not_body))) ? getTrueNode() : getFalseNode();
             }
             else if (name == Language::And && is_not_body)
@@ -391,8 +403,8 @@ namespace Ark::internal
             }
             else if (name == "len")
             {
-                if (node.list().size() > 2)
-                    throwMacroProcessingError(fmt::format("When expanding `len' inside a macro, got {} arguments, expected 1", argcount), node);
+                checkMacroArgCountEq(node, 1, "len", true);
+
                 if (Node& lst = node.list()[1]; lst.nodeType() == NodeType::List)  // only apply len at compile time if we can
                 {
                     if (isConstEval(lst))
@@ -406,8 +418,8 @@ namespace Ark::internal
             }
             else if (name == "empty?")
             {
-                if (node.list().size() > 2)
-                    throwMacroProcessingError(fmt::format("When expanding `empty?' inside a macro, got {} arguments, expected 1", argcount), node);
+                checkMacroArgCountEq(node, 1, "empty?", true);
+
                 if (Node& lst = node.list()[1]; lst.nodeType() == NodeType::List && isConstEval(lst))
                 {
                     // only apply len at compile time if we can
@@ -454,8 +466,8 @@ namespace Ark::internal
             }
             else if (name == "head")
             {
-                if (node.list().size() > 2)
-                    throwMacroProcessingError(fmt::format("When expanding `head' inside a macro, got {} arguments, expected 1", argcount), node);
+                checkMacroArgCountEq(node, 1, "head", true);
+
                 if (node.list()[1].nodeType() == NodeType::List)
                 {
                     Node& sublist = node.list()[1];
@@ -477,8 +489,8 @@ namespace Ark::internal
             }
             else if (name == "tail")
             {
-                if (node.list().size() > 2)
-                    throwMacroProcessingError(fmt::format("When expanding `tail' inside a macro, got {} arguments, expected 1", argcount), node);
+                checkMacroArgCountEq(node, 1, "tail", true);
+
                 if (node.list()[1].nodeType() == NodeType::List)
                 {
                     Node sublist = node.list()[1];
@@ -555,6 +567,8 @@ namespace Ark::internal
             }
             else if (name == Language::Argcount)
             {
+                checkMacroArgCountEq(node, 1, Language::Argcount.data(), true);
+
                 const Node sym = node.constList()[1];
                 if (sym.nodeType() == NodeType::Symbol)
                 {
@@ -570,6 +584,8 @@ namespace Ark::internal
             }
             else if (name == Language::Repr)
             {
+                checkMacroArgCountEq(node, 1, Language::Repr.data(), true);
+
                 const Node arg = node.constList()[1];
                 node.updateValueAndType(Node(NodeType::String, arg.repr()));
             }