Add extra checks on next tables (#1287)

* Add checks on the contents of the "next_tables" value in tables
* More notes in JSON_format.md on how p4c creates JSON files
* Added notes that action_id and action_data are required fields ...
inside of default_entry value.
* Add a note in JSON docs about when p4c include default_entry key for a table
* Update JSON docs making it clearer that action_id and action_data are required
* Bump max_minor_version to 24
* Change version to 2.24 in JSON format doc

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

docs/JSON_format.md

@@ -10,7 +10,7 @@ on each attribute.
 
 ## Current bmv2 JSON format version
 
-The version described in this document is *2.23*.
+The version described in this document is *2.24*.
 
 The major version number will be increased by the compiler only when
 backward-compatibility of the JSON format is broken. After a major version
@@ -625,23 +625,23 @@ attributes for these objects are:
   - `actions`: the list of actions (order does not matter) supported by this
   table
   - `next_tables`: maps each action to a next table name. Alternatively, maps
-  special string `__HIT__` and `__MISS__` to a next table name.
+  special string `__HIT__` and `__MISS__` to a next table name. See Note 3 below.
   - `direct_meters`: the name of the associated direct meter array, or null if
   the match table has no associated meter array
   - `default_entry`: an optional JSON item which can force the default entry for
   the table to be set when loading the JSON, without intervention from the
   control plane. It has the following attributes:
-    - `action_id`: the id of the default action
+    - `action_id`: the id of the default action.  Required.
     - `action_const`: an optional boolean value which is `true` iff the control
     plane is not allowed to change the default action function. Default value is
-    `false`. It can only be set to `true` for `simple` tables.
-    - `action_data`: an optional JSON array where each entry is the hexstring
+    `false`. It can only be set to `true` for `simple` tables.  See Note 2 below.
+    - `action_data`: a required JSON array where each entry is the hexstring
     value for an action argument. The size of the array needs to match the
     number of parameters expected by the action function with id `action_id`.
     - `action_entry_const`: an optional boolean value which is `true` iff the
     control plane is not allowed to modify the action entry (action function +
     action data). Default value is `false`. This attribute is ignored if the
-    `action_data` attribute it missing.
+    `action_data` attribute it missing.  See Note 2 below.
   - `entries`: enables you to optionally specify match-action entries for this
   table. Specifying entries in the JSON makes the table immutable, which means
   the added entries cannot be modified / deleted and that new entries cannot be
@@ -685,6 +685,33 @@ a miss every time it is applied, and execute its default action.  A
 dummy table has a const default action that is equal to the action `a`
 in the original source code that it is replacing.
 
+Note 2: Since May 2017 when [PR
+#653](https://github.com/p4lang/p4c/pull/653) was merged into p4c, p4c
+has always created tables with the value of `action_entry_const` equal
+to `action_const`.  They are both true if the `default_action` in the
+P4 source code for the table is declared `const`, and both false if
+the `default_action` is not declared `const`.
+Also since 2017 and perhaps earlier, p4c has always included the key
+`default_entry` in all table definitions with `type` equal to `simple`.
+Since then it has _not_ included the key `default_entry` in table
+definitions with types that were not `simple` (i.e. tables with
+action profiles or action selectors).
+
+Note 3: p4c always creates the value of the `next_tables` key in one
+of these ways:
++ If you use the P4 constructs `t1.apply().hit` or `t1.apply().miss`,
+  and use that Boolean value to choose between two execution paths,
+  e.g. in an `if` statement, then the table's `next_tables` value will
+  contain the keys `__HIT__` and/or `__MISS__` to specify these two
+  paths, and no other keys will be present.
++ If you do not use those P4 constructs, then the `next_tables` value
+  will contain keys equal to the action names of the table.  If the P4
+  program invokes the table using `switch (t1.apply().action_run)`,
+  then in general the different action names can specify different
+  next nodes to execute next, after the table is applied.  If you do
+  not use that construct, then the next node to be executed will be
+  the same for all actions.
+
 The `match_type` for the table needs to follow the following rules:
 - If one match field is `range`, the table `match_type` has to be `range`
 - If one match field is `ternary`, the table `match_type` has to be `ternary`

include/bm/bm_sim/P4Objects.h

@@ -378,6 +378,10 @@ class P4Objects {
   void init_meter_arrays(const Json::Value &root, InitState *);
   void init_register_arrays(const Json::Value &root);
   void init_actions(const Json::Value &root);
+  void check_next_nodes(const Json::Value &cfg_next_nodes,
+                        const Json::Value &cfg_actions,
+                        const std::string &table_name,
+                        bool *next_is_hit_miss);
   void init_pipelines(const Json::Value &root, LookupStructureFactory *,
                       InitState *);
   void init_checksums(const Json::Value &root);

src/bm_sim/P4Objects.cpp

@@ -44,7 +44,7 @@ using std::string;
 namespace {
 
 constexpr int required_major_version = 2;
-constexpr int max_minor_version = 23;
+constexpr int max_minor_version = 24;
 // not needed for now
 // constexpr int min_minor_version = 0;
 
@@ -1575,6 +1575,57 @@ std::vector<MatchKeyParam> parse_match_key(
 
 }  // namespace
 
+void
+P4Objects::check_next_nodes(const Json::Value &cfg_next_nodes,
+                            const Json::Value &cfg_actions,
+                            const std::string &table_name,
+                            bool *next_is_hit_miss) {
+  // For each table, the value of its key "next_tables" must be an
+  // object with one of the following sets of keys:
+  // (a) The set of keys must include "__HIT__" and "__MISS__", but no
+  //     others.  This is how a P4 table is implemented if, where it
+  //     is applied, it uses "t1.apply().hit" or "t1.apply().miss"
+  //     conditions to control which code is executed next.
+  // (b) The set of keys must include each action name of the table
+  //     exactly once, but no others.  This is how a P4 table is
+  //     implemented if where it is applied, it uses
+  //     "t1.apply().action_run" and a switch statement to control
+  //     which code is executed next.  It is also used by the p4c BMv2
+  //     backend for tables that use none of .hit, .miss, and
+  //     .action_run, and always execute the same code next regardless
+  //     of hit, miss, or which action the table executed.  In that
+  //     case, every action will have the same next node to execute
+  //     regardless of the action.
+  int num_next_nodes = cfg_next_nodes.size();
+  bool next_has_hit = cfg_next_nodes.isMember("__HIT__");
+  bool next_has_miss = cfg_next_nodes.isMember("__MISS__");
+  if (next_has_hit || next_has_miss) {
+    if (next_has_hit && next_has_miss && (num_next_nodes == 2)) {
+      *next_is_hit_miss = true;
+    } else {
+      throw json_exception(
+          EFormat() << "Table '" << table_name << "' has one"
+          << " of keys '__HIT__' and '__MISS__' in 'next_tables'"
+          << " but either it does not have both of them,"
+          << " or it has other keys that should not be there.",
+          cfg_next_nodes);
+    }
+  } else {
+    *next_is_hit_miss = false;
+    int num_actions = cfg_actions.size();
+    // The check that each action name is a key in cfg_next_nodes is
+    // done near where check_next_nodes is called, to avoid
+    // duplicating here the code that calculates action_name.
+    if (num_next_nodes != num_actions) {
+      throw json_exception(
+          EFormat() << "Table '" << table_name << "' should have exactly "
+          << num_actions << " keys, one for each table action, but found "
+          << num_next_nodes << "keys.",
+          cfg_next_nodes);
+    }
+  }
+}
+
 void
 P4Objects::init_pipelines(const Json::Value &cfg_root,
                           LookupStructureFactory *lookup_factory,
@@ -1818,6 +1869,9 @@ P4Objects::init_pipelines(const Json::Value &cfg_root,
       std::string actions_key = cfg_table.isMember("action_ids") ? "action_ids"
           : "actions";
       const Json::Value &cfg_actions = cfg_table[actions_key];
+      bool next_is_hit_miss = false;
+      check_next_nodes(cfg_next_nodes, cfg_actions, table_name,
+                       &next_is_hit_miss);
       for (const auto &cfg_action : cfg_actions) {
         p4object_id_t action_id = 0;
         string action_name = "";
@@ -1831,19 +1885,24 @@ P4Objects::init_pipelines(const Json::Value &cfg_root,
           action = get_one_action_with_name(action_name); assert(action);
           action_id = action->get_id();
         }
-
+        if (!next_is_hit_miss && !cfg_next_nodes.isMember(action_name)) {
+          throw json_exception(
+              EFormat() << "Table '" << table_name << "' should have key"
+              << " for action '" << action_name
+              << "' in its 'next_tables' object.",
+              cfg_next_nodes);
+        }
         const Json::Value &cfg_next_node = cfg_next_nodes[action_name];
         const ControlFlowNode *next_node = get_next_node(cfg_next_node);
         table->set_next_node(action_id, next_node);
         add_action_to_table(table_name, action_name, action);
         if (act_prof_name != "")
           add_action_to_act_prof(act_prof_name, action_name, action);
       }
-
-      if (cfg_next_nodes.isMember("__HIT__"))
-        table->set_next_node_hit(get_next_node(cfg_next_nodes["__HIT__"]));
-      if (cfg_next_nodes.isMember("__MISS__"))
-        table->set_next_node_miss(get_next_node(cfg_next_nodes["__MISS__"]));
+      if (next_is_hit_miss) {
+          table->set_next_node_hit(get_next_node(cfg_next_nodes["__HIT__"]));
+          table->set_next_node_miss(get_next_node(cfg_next_nodes["__MISS__"]));
+      }
 
       if (cfg_table.isMember("base_default_next")) {
         table->set_next_node_miss_default(
@@ -1888,6 +1947,11 @@ P4Objects::init_pipelines(const Json::Value &cfg_root,
 
           table->set_default_default_entry(action, std::move(adata),
                                            is_action_entry_const);
+        } else {
+          throw json_exception(
+              EFormat() << "'default_entry' of table '" << table_name
+              << "' should have key 'action_data'",
+              cfg_default_entry);
         }
       }
 
@@ -1951,9 +2015,14 @@ P4Objects::init_pipelines(const Json::Value &cfg_root,
       auto conditional_name = cfg_conditional["name"].asString();
       auto conditional = get_conditional(conditional_name);
 
+      if (!cfg_conditional.isMember("true_next") &&
+          !cfg_conditional.isMember("false_next")) {
+        throw json_exception("conditional must have either or both of the"
+                             " keys 'true_next' and 'false_next'.",
+                             cfg_conditional);
+      }
       const auto &cfg_true_next = cfg_conditional["true_next"];
       const auto &cfg_false_next = cfg_conditional["false_next"];
-
       if (!cfg_true_next.isNull()) {
         auto next_node = get_control_node_cfg(cfg_true_next.asString());
         conditional->set_next_node_if_true(next_node);