Support lvalue expressions

The bmv2 expression engine can now returns references to bmv2 primitive objects (Data, Header, ...) when the expression being evaluated is an lvalue. This means that we can now handle assignments to complex lvalue expressions which do not statically resolve to a field / header / union. One example would be an assignment to a header stack field accessed through a runtime index. Unit tests were added, although at the moment they do not support all the possible cases (e.g. expressions which evaluate to a header union). The JSON version number was bumped up to 2.23. Even though no additions were made to the format, we now support additional cases and the documentation now includes an example of a "complex" assignment statement. Fixes #838
p4lang · Dec 31, 2019 · bdbe514 · bdbe514
1 parent 14f705b
commit bdbe514
Show file tree

Hide file tree

Showing 10 changed files with 695 additions and 365 deletions.
diff --git a/docs/JSON_format.md b/docs/JSON_format.md
@@ -10,7 +10,7 @@ on each attribute.
 
 ## Current bmv2 JSON format version
 
-The version described in this document is *2.22*.
+The version described in this document is *2.23*.
 
 The major version number will be increased by the compiler only when
 backward-compatibility of the JSON format is broken. After a major version
@@ -464,6 +464,65 @@ call, with the following attributes:
     of action) array. If `type` is `extern`, this is the name of the extern
     instance. See [here](#the-type-value-object) for other types.
 
+An `expression` can either correspond to an lvalue expression or an rvalue
+expression. For example, the P4 expression `hdr.h2[hdr.h1.idx].v = hdr.h1.v + 7`
+corresponds to the following JSON object (which would be an entry in the
+`primitives` JSON array for the action to which the expression belongs):
+```json
+{
+    "op" : "assign",
+    "parameters" : [
+        {
+            "type" : "expression",
+            "value": {
+                "type": "expression",
+                "value": {
+                    "op": "access_field",
+                    "left": {
+                        "type": "expression",
+                        "value": {
+                            "op": "dereference_header_stack",
+                            "left": {
+                                "type": "header_stack",
+                                "value": "h2"
+                            },
+                            "right": {
+                                "type": "field",
+                                "value": ["h1", "idx"]
+                            }
+                        }
+                    },
+                    "right": 0
+                }
+            }
+        },
+        {
+            "type" : "expression",
+            "value": {
+                "type": "expression",
+                "value": {
+                    "op": "+",
+                    "left": {
+                        "type" : "field",
+                        "value" : ["h1", "v"]
+                    },
+                    "right": {
+                        "type" : "hexstr",
+                        "value" : "0x0000004d"
+                    }
+                }
+            }
+        }
+    ]
+}
+```
+In this case, the left-hand side of the assignment (`hdr.h2[hdr.h1.idx].v`) is
+an lvalue, while the right-hand side is an rvalue (`hdr.h1.v + 77`). However,
+this information does not need to appear *explicitly* in the JSON. The bmv2
+implementation of the `assign` primitive requires the first parameter to be a
+lvalue expression, and we assume that the compiler will not generate a JSON that
+violates this requirement. An invalid JSON will lead to undefined behavior.
+
 *Important note about extern instance methods*: even though in P4 these are
 invoked using object-oriented style, bmv2 treats them as regular primitives for
 which the first parameter is the extern instance. For example, if the P4 call is

diff --git a/include/bm/bm_sim/P4Objects.h b/include/bm/bm_sim/P4Objects.h
@@ -92,8 +92,6 @@ class P4Objects {
     std::set<std::string> headers;
   };
 
-  enum class ExprType;  // forward declaration
-
  public:
   // NOLINTNEXTLINE(runtime/references)
   explicit P4Objects(std::ostream &outstream, bool verbose_output = false);

diff --git a/include/bm/bm_sim/actions.h b/include/bm/bm_sim/actions.h
@@ -1,4 +1,5 @@
-/* Copyright 2013-present Barefoot Networks, Inc.
+/* Copyright 2013-2019 Barefoot Networks, Inc.
+ * Copyright 2019 VMware, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,7 +15,7 @@
  */
 
 /*
- * Antonin Bas ([email protected])
+ * Antonin Bas
  *
  */
 
@@ -175,7 +176,8 @@ struct ActionParam {
         HEADER_STACK, LAST_HEADER_STACK_FIELD,
         CALCULATION,
         METER_ARRAY, COUNTER_ARRAY, REGISTER_ARRAY,
-        EXPRESSION,
+        EXPRESSION, EXPRESSION_HEADER, EXPRESSION_HEADER_STACK,
+        EXPRESSION_HEADER_UNION, EXPRESSION_HEADER_UNION_STACK,
         EXTERN_INSTANCE,
         STRING,
         HEADER_UNION, HEADER_UNION_STACK, PARAMS_VECTOR} tag;
@@ -234,17 +236,17 @@ struct ActionParam {
     RegisterArray *register_array;
 
     struct {
-      unsigned int offset;
+      unsigned int offset;  // only used for arithmetic ("Data") expressions
       // non owning pointer
       // in theory, could be an owning pointer, but the union makes this
       // complicated, so instead the ActionFn keeps a vector of owning pointers
-      ArithExpression *ptr;
+      Expression *ptr;
     } expression;
 
     ExternType *extern_instance;
 
     // I use a pointer here to avoid complications with the union; the string
-    // memory is owned by ActionFn (just like for ArithExpression above)
+    // memory is owned by ActionFn (just like for Expression above)
     const std::string *str;
 
     header_union_stack_id_t header_union_stack;
@@ -273,7 +275,6 @@ struct ActionEngineState {
 // they have to be declared outside of the class declaration, and "inline" is
 // necessary to avoid linker errors
 
-// can only be a field or a register reference
 template <> inline
 Data &ActionParam::to<Data &>(ActionEngineState *state) const {
   static thread_local Data data_temp;
@@ -284,9 +285,12 @@ Data &ActionParam::to<Data &>(ActionEngineState *state) const {
     case ActionParam::REGISTER_REF:
       return register_ref.array->at(register_ref.idx);
     case ActionParam::REGISTER_GEN:
-      register_gen.idx->eval(state->phv, &data_temp,
-                             state->action_data.action_data);
+      register_gen.idx->eval_arith(state->phv, &data_temp,
+                                   state->action_data.action_data);
       return register_ref.array->at(data_temp.get<size_t>());
+    case ActionParam::EXPRESSION:
+      return expression.ptr->eval_arith_lvalue(&state->phv,
+                                               state->action_data.action_data);
     case ActionParam::LAST_HEADER_STACK_FIELD:
       return state->phv.get_header_stack(stack_field.header_stack).get_last()
           .get_field(stack_field.field_offset);
@@ -312,16 +316,16 @@ const Data &ActionParam::to<const Data &>(ActionEngineState *state) const {
     case ActionParam::REGISTER_REF:
       return register_ref.array->at(register_ref.idx);
     case ActionParam::REGISTER_GEN:
-      register_gen.idx->eval(state->phv, &data_temps[0],
-                             state->action_data.action_data);
+      register_gen.idx->eval_arith(state->phv, &data_temps[0],
+                                   state->action_data.action_data);
       return register_ref.array->at(data_temps[0].get<size_t>());
     case ActionParam::EXPRESSION:
       while (data_temps_size <= expression.offset) {
         data_temps.emplace_back();
         data_temps_size++;
       }
-      expression.ptr->eval(state->phv, &data_temps[expression.offset],
-                              state->action_data.action_data);
+      expression.ptr->eval_arith(state->phv, &data_temps[expression.offset],
+                                 state->action_data.action_data);
       return data_temps[expression.offset];
     case ActionParam::LAST_HEADER_STACK_FIELD:
       return state->phv.get_header_stack(stack_field.header_stack).get_last()
@@ -347,8 +351,15 @@ const Field &ActionParam::to<const Field &>(ActionEngineState *state) const {
 
 template <> inline
 Header &ActionParam::to<Header &>(ActionEngineState *state) const {
-  assert(tag == ActionParam::HEADER);
-  return state->phv.get_header(header);
+  switch (tag) {
+    case ActionParam::HEADER:
+      return state->phv.get_header(header);
+    case ActionParam::EXPRESSION_HEADER:
+      return expression.ptr->eval_header(&state->phv,
+                                         state->action_data.action_data);
+    default:
+      _BM_UNREACHABLE("Default switch case should not be reachable");
+  }
 }
 
 template <> inline
@@ -358,8 +369,15 @@ const Header &ActionParam::to<const Header &>(ActionEngineState *state) const {
 
 template <> inline
 HeaderStack &ActionParam::to<HeaderStack &>(ActionEngineState *state) const {
-  assert(tag == ActionParam::HEADER_STACK);
-  return state->phv.get_header_stack(header_stack);
+  switch (tag) {
+    case ActionParam::HEADER_STACK:
+      return state->phv.get_header_stack(header_stack);
+    case ActionParam::EXPRESSION_HEADER_STACK:
+      return expression.ptr->eval_header_stack(&state->phv,
+                                               state->action_data.action_data);
+    default:
+      _BM_UNREACHABLE("Default switch case should not be reachable");
+  }
 }
 
 template <> inline
@@ -373,8 +391,14 @@ StackIface &ActionParam::to<StackIface &>(ActionEngineState *state) const {
   switch (tag) {
     case HEADER_STACK:
       return state->phv.get_header_stack(header_stack);
+    case ActionParam::EXPRESSION_HEADER_STACK:
+      return expression.ptr->eval_header_stack(
+          &state->phv, state->action_data.action_data);
     case HEADER_UNION_STACK:
       return state->phv.get_header_union_stack(header_union_stack);
+    case ActionParam::EXPRESSION_HEADER_UNION_STACK:
+      return expression.ptr->eval_header_union_stack(
+          &state->phv, state->action_data.action_data);
     default:
       _BM_UNREACHABLE("Default switch case should not be reachable");
   }
@@ -388,8 +412,15 @@ const StackIface &ActionParam::to<const StackIface &>(
 
 template <> inline
 HeaderUnion &ActionParam::to<HeaderUnion &>(ActionEngineState *state) const {
-  assert(tag == ActionParam::HEADER_UNION);
-  return state->phv.get_header_union(header_union);
+  switch (tag) {
+    case ActionParam::HEADER_UNION:
+      return state->phv.get_header_union(header_union);
+    case ActionParam::EXPRESSION_HEADER_UNION:
+      return expression.ptr->eval_header_union(&state->phv,
+                                               state->action_data.action_data);
+    default:
+      _BM_UNREACHABLE("Default switch case should not be reachable");
+  }
 }
 
 template <> inline
@@ -401,8 +432,15 @@ const HeaderUnion &ActionParam::to<const HeaderUnion &>(
 template <> inline
 HeaderUnionStack &ActionParam::to<HeaderUnionStack &>(
     ActionEngineState *state) const {
-  assert(tag == ActionParam::HEADER_UNION_STACK);
-  return state->phv.get_header_union_stack(header_union_stack);
+  switch (tag) {
+    case ActionParam::HEADER_UNION_STACK:
+      return state->phv.get_header_union_stack(header_union_stack);
+    case ActionParam::EXPRESSION_HEADER_UNION_STACK:
+      return expression.ptr->eval_header_union_stack(
+          &state->phv, state->action_data.action_data);
+    default:
+      _BM_UNREACHABLE("Default switch case should not be reachable");
+  }
 }
 
 template <> inline
@@ -698,7 +736,9 @@ class ActionFn :  public NamedP4Object {
   void parameter_push_back_meter_array(MeterArray *meter_array);
   void parameter_push_back_counter_array(CounterArray *counter_array);
   void parameter_push_back_register_array(RegisterArray *register_array);
-  void parameter_push_back_expression(std::unique_ptr<ArithExpression> expr);
+  void parameter_push_back_expression(std::unique_ptr<Expression> expr);
+  void parameter_push_back_expression(std::unique_ptr<Expression> expr,
+                                      ExprType expr_type);
   void parameter_push_back_extern_instance(ExternType *extern_instance);
   void parameter_push_back_string(const std::string &str);
 
@@ -727,7 +767,7 @@ class ActionFn :  public NamedP4Object {
   RegisterSync register_sync{};
   std::vector<Data> const_values{};
   // should I store the objects in the vector, instead of pointers?
-  std::vector<std::unique_ptr<ArithExpression> > expressions{};
+  std::vector<std::unique_ptr<Expression> > expressions{};
   std::vector<std::string> strings{};
   size_t num_params;
 

diff --git a/include/bm/bm_sim/expressions.h b/include/bm/bm_sim/expressions.h
@@ -1,4 +1,5 @@
-/* Copyright 2013-present Barefoot Networks, Inc.
+/* Copyright 2013-2019 Barefoot Networks, Inc.
+ * Copyright 2019 VMware, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,7 +15,7 @@
  */
 
 /*
- * Antonin Bas ([email protected])
+ * Antonin Bas
  *
  */
 
@@ -26,13 +27,18 @@
 #include <vector>
 
 #include "data.h"
+#include "headers.h"
+#include "header_unions.h"
 #include "phv_forward.h"
+#include "stacks.h"
 
 namespace bm {
 
 class RegisterArray;
 class RegisterSync;
 
+struct ExpressionTemps;
+
 enum class ExprOpcode {
   LOAD_FIELD, LOAD_HEADER, LOAD_HEADER_STACK, LOAD_LAST_HEADER_STACK_FIELD,
   LOAD_UNION, LOAD_UNION_STACK, LOAD_BOOL, LOAD_CONST, LOAD_LOCAL,
@@ -56,6 +62,10 @@ enum class ExprOpcode {
   ACCESS_UNION_HEADER,
 };
 
+enum class ExprType {
+  UNKNOWN, DATA, HEADER, HEADER_STACK, BOOL, UNION, UNION_STACK
+};
+
 class ExprOpcodesMap {
  public:
   static ExprOpcode get_opcode(std::string expr_name);
@@ -69,6 +79,14 @@ class ExprOpcodesMap {
   std::unordered_map<std::string, ExprOpcode> opcodes_map{};
 };
 
+class ExprOpcodesUtils {
+ public:
+  static ExprOpcode get_eq_opcode(ExprType expr_type);
+  static ExprOpcode get_neq_opcode(ExprType expr_type);
+
+  static ExprType get_opcode_type(ExprOpcode opcode);
+};
+
 struct Op {
   ExprOpcode opcode;
 
@@ -122,6 +140,8 @@ class Expression {
  public:
   Expression();
 
+  virtual ~Expression() { }
+
   void push_back_load_field(header_id_t header, int field_offset);
   void push_back_load_bool(bool value);
   void push_back_load_header(header_id_t header);
@@ -149,6 +169,14 @@ class Expression {
   Data eval_arith(const PHV &phv, const std::vector<Data> &locals = {}) const;
   void eval_arith(const PHV &phv, Data *data,
                   const std::vector<Data> &locals = {}) const;
+  Data &eval_arith_lvalue(PHV *phv, const std::vector<Data> &locals = {}) const;
+  Header &eval_header(PHV *phv, const std::vector<Data> &locals = {}) const;
+  HeaderStack &eval_header_stack(
+      PHV *phv, const std::vector<Data> &locals = {}) const;
+  HeaderUnion &eval_header_union(
+      PHV *phv, const std::vector<Data> &locals = {}) const;
+  HeaderUnionStack &eval_header_union_stack(
+      PHV *phv, const std::vector<Data> &locals = {}) const;
 
   bool empty() const;
 
@@ -159,14 +187,11 @@ class Expression {
   Expression(Expression &&other) /*noexcept*/ = default;
   Expression &operator=(Expression &&other) /*noexcept*/ = default;
 
- private:
-  enum class ExprType {EXPR_BOOL, EXPR_DATA};
-
  private:
   int assign_dest_registers();
-  void eval_(const PHV &phv, ExprType expr_type,
+  void eval_(const PHV &phv,
              const std::vector<Data> &locals,
-             bool *b_res, Data *d_res) const;
+             ExpressionTemps *temps) const;
   size_t get_num_ops() const;
   void append_expression(const Expression &e);
 
@@ -186,6 +211,10 @@ class ArithExpression : public Expression {
             const std::vector<Data> &locals = {}) const {
     eval_arith(phv, data, locals);
   }
+
+  Data &eval_lvalue(PHV *phv, const std::vector<Data> &locals = {}) const {
+    return eval_arith_lvalue(phv, locals);
+  }
 };