Filter: Store variables and function arguments on stack

9 files changed, 123 insertions, 194 deletions

diff --git a/filter/config.Y b/filter/config.Y
index 3eccc3ed..5f9b8356 100644
--- a/filter/config.Y
+++ b/filter/config.Y
@@ -15,6 +15,8 @@ CF_HDR
 
 CF_DEFINES
 
+static uint decls_count;
+
 static inline u32 pair(u32 a, u32 b) { return (a << 16) | b; }
 static inline u32 pair_a(u32 p) { return p >> 16; }
 static inline u32 pair_b(u32 p) { return p & 0xFFFF; }
@@ -398,8 +400,8 @@ assert_assign(struct f_lval *lval, struct f_inst *expr, const char *start, const
   struct f_inst *setter, *getter, *checker;
   switch (lval->type) {
     case F_LVAL_VARIABLE:
-      setter = f_new_inst(FI_SET, expr, lval->sym);
-      getter = f_new_inst(FI_VARIABLE, lval->sym);
+      setter = f_new_inst(FI_VAR_SET, expr, lval->sym);
+      getter = f_new_inst(FI_VAR_GET, lval->sym);
       break;
     case F_LVAL_PREFERENCE:
       setter = f_new_inst(FI_PREF_SET, expr);
@@ -446,14 +448,14 @@ CF_KEYWORDS(FUNCTION, PRINT, PRINTN, UNSET, RETURN,
 %nonassoc THEN
 %nonassoc ELSE
 
-%type <xp> cmds_int function_body declsn function_params
-%type <x> term block cmd cmds constant constructor print_one print_list var_list var_listn function_call symbol_value bgp_path_expr bgp_path bgp_path_tail one_decl decls
+%type <xp> cmds_int
+%type <x> term block cmd cmds constant constructor print_one print_list var_list function_call symbol_value bgp_path_expr bgp_path bgp_path_tail
 %type <fda> dynamic_attr
 %type <fsa> static_attr
 %type <f> filter where_filter
-%type <fl> filter_body
+%type <fl> filter_body function_body
 %type <flv> lvalue
-%type <i> type
+%type <i> type function_params
 %type <ecs> ec_kind
 %type <fret> break_command 
 %type <i32> cnum
@@ -553,44 +555,25 @@ type:
    }
  ;
 
-one_decl:
-   type CF_SYM_VOID {
-     struct f_val * val = cfg_alloc(sizeof(struct f_val));
-     val->type = T_VOID;
-     $2 = cf_define_symbol($2, SYM_VARIABLE | $1, val, val);
-     DBG( "New variable %s type %x\n", $2->name, $1 );
-     $$ = f_new_inst(FI_SET, NULL, $2);
-   }
- ;
-
-/* Decls with ';' at the end. Beware; these are reversed. */
-decls: /* EMPTY */ { $$ = NULL; }
- | one_decl ';' decls {
-     $$ = $1;
-     $$->next = $3;
-   }
+/* Declarations with ';' at the end */
+decls:
+   /* EMPTY */
+ | declsn ';'
  ;
 
 /* Declarations that have no ';' at the end. */
-declsn: one_decl { $$[0] = $$[1] = $1; }
- | one_decl ';' declsn {
-     $3[1]->next = $1;
-     $$[1] = $3[1] = $1;
-     $$[0] = $3[0];
+declsn:
+   type CF_SYM_VOID {
+     cf_define_symbol($2, SYM_VARIABLE | $1, offset, decls_count++);
    }
- ;
-
-filter_body:
-   function_body {
-     if ($1[0]) {
-       const struct f_inst *inst[2] = { $1[0], $1[1] };
-       $$ = f_postfixify_concat(inst, 2);
-     }
-     else
-       $$ = f_postfixify($1[1]);
+ | declsn ';' type CF_SYM_VOID {
+     if (decls_count >= 0xff) cf_error("Too many declarations, at most 255 allowed");
+     cf_define_symbol($4, SYM_VARIABLE | $3, offset, decls_count++);
    }
  ;
 
+filter_body: { decls_count = 0; } function_body { $$ = $2; } ;
+
 filter:
    CF_SYM_KNOWN {
      cf_assert_symbol($1, SYM_FILTER);
@@ -611,14 +594,14 @@ where_filter:
  ;
 
 function_params:
-   '(' declsn ')' { $$[0] = $2[0]; $$[1] = $2[1]; }
- | '(' ')' { $$[0] = $$[1] = NULL; }
+   '(' declsn ')' { $$ = decls_count; }
+ | '(' ')' { $$ = 0; }
  ;
 
 function_body:
    decls '{' cmds '}' {
-     $$[0] = $1 ? f_clear_local_vars($1) : NULL;
-     $$[1] = $3;
+     $$ = f_postfixify($3);
+     $$->vars = decls_count;
    }
  ;
 
@@ -627,33 +610,11 @@ function_def:
    FUNCTION CF_SYM_VOID { DBG( "Beginning of function %s\n", $2->name );
      $2 = cf_define_symbol($2, SYM_FUNCTION, function, NULL);
      cf_push_scope($2);
+     decls_count = 0;
    } function_params function_body {
-     const struct f_inst *catlist[4];
-     uint count = 0;
-
-     /* Argument setters */
-     if ($4[0])
-       catlist[count++] = $4[0];
-
-     /* Local var clearers */
-     if ($5[0])
-       catlist[count++] = $5[0];
-
-     /* Return void if no return is needed */
-     catlist[count++] = f_new_inst(FI_CONSTANT, (struct f_val) { .type = T_VOID });
-
-     /* Function body itself */
-     if ($5[1])
-       catlist[count++] = $5[1];
-
-     struct f_line *fl = f_postfixify_concat(catlist, count);
-
-     fl->args = 0;
-     for (const struct f_inst *arg = $4[0]; arg; arg = arg->next)
-       fl->args++;
-
-     $2->function = fl;
-
+     $5->vars -= $4;
+     $5->args = $4;
+     $2->function = $5;
      cf_pop_scope();
    }
  ;
@@ -862,9 +823,33 @@ constructor:
  ;
 
 
+/* This generates the function_call variable list backwards. */
+var_list: /* EMPTY */ { $$ = NULL; }
+ | term { $$ = $1; }
+ | var_list ',' term { $$ = $3; $$->next = $1; }
+
 function_call:
    CF_SYM_KNOWN '(' var_list ')' {
-     $$ = f_new_inst(FI_CALL, $1, $3);
+     if ($1->class != SYM_FUNCTION)
+       cf_error("You can't call something which is not a function. Really.");
+
+     struct f_inst *fc = f_new_inst(FI_CALL, $1);
+     uint args = 0;
+     while ($3) {
+       args++;
+       struct f_inst *tmp = $3->next;
+       $3->next = fc;
+
+       fc = $3;
+       $3 = tmp;
+     }
+
+     if (args != $1->function->args)
+       cf_error("Function call '%s' got %u arguments, need %u arguments.",
+	   $1->name, args, $1->function->args);
+
+     $$ = f_new_inst(FI_CONSTANT, (struct f_val) { .type = T_VOID });
+     $$->next = fc;
    }
  ;
 
@@ -872,8 +857,10 @@ symbol_value: CF_SYM_KNOWN
   {
     switch ($1->class) {
       case SYM_CONSTANT_RANGE:
+	$$ = f_new_inst(FI_CONSTANT_DEFINED, $1);
+	break;
       case SYM_VARIABLE_RANGE:
-	$$ = f_new_inst(FI_VARIABLE, $1);
+	$$ = f_new_inst(FI_VAR_GET, $1);
 	break;
       case SYM_ATTRIBUTE:
 	$$ = f_new_inst(FI_EA_GET, *$1->attribute);
@@ -988,19 +975,6 @@ print_list: /* EMPTY */ { $$ = NULL; }
    }
  ;
 
-var_listn: term {
-     $$ = $1;
-   }
- | term ',' var_listn {
-     $$ = $1;
-     $$->next = $3;
-   }
- ;
-
-var_list: /* EMPTY */ { $$ = NULL; }
- | var_listn { $$ = $1; }
- ;
-
 cmd:
    IF term THEN block {
      $$ = f_new_inst(FI_CONDITION, $2, $4, NULL);
@@ -1011,7 +985,7 @@ cmd:
  | CF_SYM_KNOWN '=' term ';' {
      switch ($1->class) {
        case SYM_VARIABLE_RANGE:
-	 $$ = f_new_inst(FI_SET, $3, $1);
+	 $$ = f_new_inst(FI_VAR_SET, $3, $1);
 	 break;
        case SYM_ATTRIBUTE:
 	 $$ = f_new_inst(FI_EA_SET, $3, *$1->attribute);
diff --git a/filter/data.h b/filter/data.h
index 6ef2024f..6973008f 100644
--- a/filter/data.h
+++ b/filter/data.h
@@ -17,8 +17,8 @@
 
 /* Internal types */
 enum f_type {
-/* Do not use type of zero, that way we'll see errors easier. */
-  T_VOID = 1,
+/* Nothing. Simply nothing. */
+  T_VOID = 0,
 
 /* User visible types, which fit in int */
   T_INT = 0x10,
diff --git a/filter/decl.m4 b/filter/decl.m4
index bdd59f20..c9b5c8c5 100644
--- a/filter/decl.m4
+++ b/filter/decl.m4
@@ -209,6 +209,7 @@ do {
   estk.item[estk.cnt].pos = 0;
   estk.item[estk.cnt].line = $1;
   estk.item[estk.cnt].ventry = vstk.cnt;
+  estk.item[estk.cnt].vbase = estk.item[estk.cnt-1].vbase;
   estk.item[estk.cnt].emask = 0;
   estk.cnt++;
 } while (0)m4_dnl
@@ -377,7 +378,7 @@ FID_WR_PUT(4)
 
 /* Filter instruction structure for config */
 struct f_inst {
-  const struct f_inst *next;		/* Next instruction */
+  struct f_inst *next;			/* Next instruction */
   enum f_instruction_code fi_code;	/* Instruction code */
   int size;				/* How many instructions are underneath */
   int lineno;				/* Line number */
diff --git a/filter/f-inst.c b/filter/f-inst.c
index d6c292b6..4a4f6016 100644
--- a/filter/f-inst.c
+++ b/filter/f-inst.c
@@ -261,23 +261,28 @@
   }
 
   /* Set to indirect value prepared in v1 */
-  INST(FI_SET, 1, 0) {
+  INST(FI_VAR_SET, 1, 0) {
     ARG_ANY(2);
     SYMBOL(1);
     if ((sym->class != (SYM_VARIABLE | v1.type)) && (v1.type != T_VOID))
     {
       /* IP->Quad implicit conversion */
       if ((sym->class == (SYM_VARIABLE | T_QUAD)) && val_is_ip4(&v1))
-      {
-	*(sym->val) = (struct f_val) {
+	v1 = (struct f_val) {
 	  .type = T_QUAD,
 	  .val.i = ipa_to_u32(v1.val.ip),
-	}; 
-	break;
-      }
-      runtime( "Assigning to variable of incompatible type" );
+	};
+      else 
+	runtime( "Assigning to variable of incompatible type" );
     }
-    *(sym->val) = v1;
+
+    vstk.val[curline.vbase + sym->offset] = v1;
+  }
+
+  INST(FI_VAR_GET, 0, 1) {
+    SYMBOL(1);
+    res = vstk.val[curline.vbase + sym->offset];
+    RESULT_OK;
   }
 
     /* some constants have value in a[1], some in *a[0].p, strange. */
@@ -301,7 +306,7 @@
     res = whati->val;
     RESULT_OK;
   }
-  INST(FI_VARIABLE, 0, 1) {
+  INST(FI_CONSTANT_DEFINED, 0, 1) {
     FID_STRUCT_IN
       const struct symbol *sym;
     FID_LINE_IN
@@ -314,18 +319,9 @@
     FID_POSTFIXIFY_BODY
       item->valp = (item->sym = what->sym)->val;
     FID_SAME_BODY
-      if (strcmp(f1->sym->name, f2->sym->name) || (f1->sym->class != f2->sym->class)) return 0;
+      if (strcmp(f1->sym->name, f2->sym->name) || !val_same(f1->sym->val, f2->sym->val)) return 0;
     FID_DUMP_BODY
-      switch (item->sym->class) {
-	case SYM_CONSTANT_RANGE:
-	  debug("%sconstant %s with value %s\n", INDENT, item->sym->name, val_dump(item->valp));
-	  break;
-	case SYM_VARIABLE_RANGE:
-	  debug("%svariable %s with current value %s\n", INDENT, item->sym->name, val_dump(item->valp));
-	  break;
-	default:
-	  bug("Symbol %s of type %d doesn't reference a value", item->sym->name, item->sym->class);
-      }
+      debug("%sconstant %s with value %s\n", INDENT, item->sym->name, val_dump(item->valp));
     FID_ALL
 
     res = *whati->valp;
@@ -768,56 +764,31 @@
       else
 	runtime("Can't return non-bool from non-function");
 
-    /* Set the value stack position */
-    vstk.cnt = estk.item[estk.cnt].ventry;
+    /* Set the value stack position, overwriting the former implicit void */
+    vstk.cnt = estk.item[estk.cnt].ventry - 1;
 
     /* Copy the return value */
     RESULT_VAL(vstk.val[retpos]);
   }
 
   INST(FI_CALL, 0, 1) {
-    FID_LINE_IN
-      const struct f_line *args;
-      const struct f_line *body;
-      struct symbol *sym;
-    FID_STRUCT_IN
-      struct symbol *sym;
-      const struct f_inst *args;
-    FID_NEW_ARGS
-      , struct symbol * sym
-      , const struct f_inst *args
-    FID_NEW_BODY
-      if (sym->class != SYM_FUNCTION)
-	cf_error("You can't call something which is not a function. Really.");
-
-      uint count = 0;
-      for (const struct f_inst *inst = args; inst; inst = inst->next)
-	count++;
+    SYMBOL;
 
-      if (count != sym->function->args)
-	cf_error("Function %s takes %u arguments, got %u.", sym->name, sym->function->args, count);
-
-      what->sym = sym;
-      what->args = args;
-    FID_DUMP_BODY
-      debug("%scalling %s with following args\n", INDENT, item->sym->name);
-      f_dump_line(item->args, indent + 1);
-    FID_POSTFIXIFY_BODY
-      item->args = f_postfixify(what->args);
-      item->body = (item->sym = what->sym)->function;
-    FID_SAME_BODY
-      /* To be done better */
-      if (strcmp(f1->sym->name, f2->sym->name)) return 0;
-      if (!f_same(f1->args, f2->args)) return 0;
-      if (!f_same(f1->body, f2->body)) return 0;
-    FID_ALL
-
-    /* First push the body on stack */
-    LINEX(whati->body);
+    /* Push the body on stack */
+    LINEX(sym->function);
     curline.emask |= FE_RETURN;
-
-    /* Then push the arguments */
-    LINEX(whati->args);
+  
+    /* Before this instruction was called, there was the T_VOID
+     * automatic return value pushed on value stack and also
+     * sym->function->args function arguments. Setting the
+     * vbase to point to first argument. */
+    ASSERT(curline.ventry >= sym->function->args);
+    curline.ventry -= sym->function->args;
+    curline.vbase = curline.ventry;
+
+    /* Storage for local variables */
+    memset(&(vstk.val[vstk.cnt]), 0, sizeof(struct f_val) * sym->function->vars);
+    vstk.cnt += sym->function->vars;
   }
 
   INST(FI_DROP_RESULT, 1, 0) {
diff --git a/filter/f-inst.h b/filter/f-inst.h
index 1e2d63a2..21cec454 100644
--- a/filter/f-inst.h
+++ b/filter/f-inst.h
@@ -63,13 +63,12 @@ enum f_instruction_flags {
 /* Convert the instruction back to the enum name */
 const char *f_instruction_name(enum f_instruction_code fi);
 
-struct f_inst *f_clear_local_vars(struct f_inst *decls);
-
 /* Filter structures for execution */
 /* Line of instructions to be unconditionally executed one after another */
 struct f_line {
   uint len;				/* Line length */
   u8 args;				/* Function: Args required */
+  u8 vars;
   struct f_line_item items[0];		/* The items themselves */
 };
 
diff --git a/filter/f-util.c b/filter/f-util.c
index 35944b2c..79201fba 100644
--- a/filter/f-util.c
+++ b/filter/f-util.c
@@ -30,11 +30,6 @@ filter_name(const struct filter *filter)
     return filter->sym->name;
 }
 
-void f_inst_next(struct f_inst *first, const struct f_inst *append)
-{
-  first->next = append;
-}
-
 struct filter *f_new_where(const struct f_inst *where)
 {
   struct f_inst acc = {
@@ -67,23 +62,6 @@ struct filter *f_new_where(const struct f_inst *where)
   return f;
 }
 
-struct f_inst *f_clear_local_vars(struct f_inst *decls)
-{
-  /* Prepend instructions to clear local variables */
-  struct f_inst *head = NULL;
-
-  for (const struct f_inst *si = decls; si; si = si->next) {
-    struct f_inst *cur = f_new_inst(FI_CONSTANT, (struct f_val) { .type = T_VOID });
-    if (head)
-      f_inst_next(cur, head);
-    else
-      f_inst_next(cur, si);
-    head = cur;	/* The first FI_CONSTANT put there */
-  }
-
-  return head;
-}
-
 #define CA_KEY(n)	n->name, n->fda.type
 #define CA_NEXT(n)	n->next
 #define CA_EQ(na,ta,nb,tb)	(!strcmp(na,nb) && (ta == tb))
diff --git a/filter/filter.c b/filter/filter.c
index 50d9414b..dbc2376b 100644
--- a/filter/filter.c
+++ b/filter/filter.c
@@ -135,6 +135,8 @@ static struct tbf rl_runtime_err = TBF_DEFAULT_LOG_LIMITS;
 static enum filter_return
 interpret(struct filter_state *fs, const struct f_line *line, struct f_val *val)
 {
+  /* No arguments allowed */
+  ASSERT(line->args == 0);
 
 #define F_VAL_STACK_MAX	4096
   /* Value stack for execution */
@@ -145,8 +147,12 @@ interpret(struct filter_state *fs, const struct f_line *line, struct f_val *val)
 
   /* The stack itself is intentionally kept as-is for performance reasons.
    * Do NOT rewrite this to initialization by struct literal. It's slow.
-   */
-  vstk.cnt = 0;
+   *
+   * Reserving space for local variables. */
+
+  vstk.cnt = line->vars;
+  memset(vstk.val, 0, sizeof(struct f_val) * line->vars);
+
 #define F_EXEC_STACK_MAX 4096
 
   /* Exception bits */
@@ -160,6 +166,7 @@ interpret(struct filter_state *fs, const struct f_line *line, struct f_val *val)
       const struct f_line *line;		/* The line that is being executed */
       uint pos;				/* Instruction index in the line */
       uint ventry;			/* Value stack depth on entry */
+      uint vbase;			/* Where to index variable positions from */
       enum f_exception emask;		/* Exception mask */
     } item[F_EXEC_STACK_MAX];
     uint cnt;				/* Current stack size; 0 for empty */
@@ -181,7 +188,6 @@ interpret(struct filter_state *fs, const struct f_line *line, struct f_val *val)
     while (curline.pos < curline.line->len) {
       const struct f_line_item *what = &(curline.line->items[curline.pos++]);
 
-
       switch (what->fi_code) {
 #define res vstk.val[vstk.cnt]
 #define v1 vstk.val[vstk.cnt]
@@ -207,26 +213,28 @@ interpret(struct filter_state *fs, const struct f_line *line, struct f_val *val)
 #undef ACCESS_EATTRS
       }
     }
+    
+    /* End of current line. Drop local variables before exiting. */
+    vstk.cnt -= curline.line->vars;
+    vstk.cnt -= curline.line->args;
     estk.cnt--;
   }
 
-  switch (vstk.cnt) {
-    case 0:
-      if (val) {
-	log_rl(&rl_runtime_err, L_ERR "filters: No value left on stack");
-	return F_ERROR;
-      }
-      return F_NOP;
-    case 1:
-      if (val) {
-	*val = vstk.val[0];
-	return F_NOP;
-      }
-      /* fallthrough */
-    default:
-      log_rl(&rl_runtime_err, L_ERR "Too many items left on stack: %u", vstk.cnt);
+  if (vstk.cnt == 0) {
+    if (val) {
+      log_rl(&rl_runtime_err, L_ERR "filters: No value left on stack");
       return F_ERROR;
+    }
+    return F_NOP;
   }
+
+  if (val && (vstk.cnt == 1)) {
+    *val = vstk.val[0];
+    return F_NOP;
+  }
+
+  log_rl(&rl_runtime_err, L_ERR "Too many items left on stack: %u", vstk.cnt);
+  return F_ERROR;
 }
 
 
diff --git a/filter/filter_test.c b/filter/filter_test.c
index d0dd281a..3abe095b 100644
--- a/filter/filter_test.c
+++ b/filter/filter_test.c
@@ -56,8 +56,7 @@ run_function(const void *arg)
   }
 
   linpool *tmp = lp_new_default(&root_pool);
-  struct f_val res;
-  enum filter_return fret = f_eval(t->fn, tmp, &res);
+  enum filter_return fret = f_eval(t->fn, tmp, NULL);
   rfree(tmp);
 
   return (fret < F_REJECT);
diff --git a/filter/test.conf b/filter/test.conf
index ba25a34b..9abd76f3 100644
--- a/filter/test.conf
+++ b/filter/test.conf
@@ -7,8 +7,7 @@
 router id 62.168.0.1;
 
 /* We have to setup any protocol */
-protocol static { ipv4; }
-
+protocol device { }