Removed statements with side effects from asserts (#402)

In release builds, it is common to define NDEBUG to ignore
asserts. Which means it is a bad idea to call methods with necessary
side-effects inside asserts...

include/Makefile.am

@@ -18,6 +18,7 @@ bm/bm_runtime/bm_runtime.h
 endif
 
 nobase_include_HEADERS += \
+bm/bm_sim/_assert.h \
 bm/bm_sim/action_entry.h \
 bm/bm_sim/action_profile.h \
 bm/bm_sim/actions.h \

include/bm/bm_sim/_assert.h

@@ -0,0 +1,39 @@
+/* Copyright 2013-present Barefoot Networks, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * Antonin Bas ([email protected])
+ *
+ */
+
+#ifndef BM_BM_SIM__ASSERT_H_
+#define BM_BM_SIM__ASSERT_H_
+
+// An assert that cannot be removed with NDEBUG
+
+namespace bm {
+
+[[ noreturn ]] void _assert(const char* expr, const char* file, int line);
+
+}  // namespace bm
+
+#define _BM_ASSERT(expr) \
+  ((expr) ? (void)0 : bm::_assert(#expr, __FILE__, __LINE__))
+
+#define _BM_UNREACHABLE(msg) bm::_assert(msg, __FILE__, __LINE__)
+
+#define _BM_UNUSED(x) ((void)x)
+
+#endif  // BM_BM_SIM__ASSERT_H_

include/bm/bm_sim/actions.h

@@ -84,6 +84,7 @@
 #include "named_p4object.h"
 #include "expressions.h"
 #include "stateful.h"
+#include "_assert.h"
 
 namespace bm {
 
@@ -278,7 +279,7 @@ Data &ActionParam::to<Data &>(ActionEngineState *state) const {
       return state->phv.get_header_stack(stack_field.header_stack).get_last()
           .get_field(stack_field.field_offset);
     default:
-      assert(0);
+      _BM_UNREACHABLE("Default switch case should not be reachable");
   }
 }
 
@@ -314,7 +315,7 @@ const Data &ActionParam::to<const Data &>(ActionEngineState *state) const {
       return state->phv.get_header_stack(stack_field.header_stack).get_last()
           .get_field(stack_field.field_offset);
     default:
-      assert(0);
+      _BM_UNREACHABLE("Default switch case should not be reachable");
   }
 }
 
@@ -363,7 +364,7 @@ StackIface &ActionParam::to<StackIface &>(ActionEngineState *state) const {
     case HEADER_UNION_STACK:
       return state->phv.get_header_union_stack(header_union_stack);
     default:
-      assert(0);
+      _BM_UNREACHABLE("Default switch case should not be reachable");
   }
 }
 

include/bm/bm_sim/nn.h

@@ -95,6 +95,7 @@ namespace nn
       inline ~socket ()
       {
 	int rc = nn_close (s);
+        (void) rc;
 	assert (rc == 0);
       }
 

src/bf_lpm_trie/bf_lpm_trie.c

@@ -32,6 +32,8 @@ typedef unsigned char byte_t;
 
 typedef unsigned long value_t;
 
+#define _unused(x) ((void)(x))
+
 typedef struct node_s {
   Pvoid_t PJLarray_branches;
   Pvoid_t PJLarray_prefixes;
@@ -270,14 +272,18 @@ bool bf_lpm_trie_delete(bf_lpm_trie_t *trie, const char *prefix,
   pdata = get_prefix_ptr(current_node, prefix_key);
   if(!pdata) return false;
 
+  int success;
+  _unused(success);
   if(trie->release_memory) {
-    assert(delete_prefix(current_node, prefix_key) == 1);
+    success = delete_prefix(current_node, prefix_key);
+    assert(success == 1);
     current_node->pref_num--;
     while(current_node->pref_num == 0 && current_node->branch_num == 0) {
       node_t *tmp = current_node;
       current_node = current_node->parent;
       if(!current_node) break;
-      assert(delete_branch(current_node, tmp->child_id) == 1);
+      success = delete_branch(current_node, tmp->child_id);
+      assert(success == 1);
       free(tmp);
       current_node->branch_num--;
     }
@@ -286,4 +292,4 @@ bool bf_lpm_trie_delete(bf_lpm_trie_t *trie, const char *prefix,
   return true;
 }
 
-
+#undef _unused

src/bm_apps/nn.h

@@ -95,6 +95,7 @@ namespace nn
       inline ~socket ()
       {
 	int rc = nn_close (s);
+        (void) rc;
 	assert (rc == 0);
       }
 

src/bm_runtime/Standard_server.cpp

@@ -20,8 +20,9 @@
 
 #include <bm/Standard.h>
 
-#include <bm/bm_sim/switch.h>
+#include <bm/bm_sim/_assert.h>
 #include <bm/bm_sim/logger.h>
+#include <bm/bm_sim/switch.h>
 
 #include <functional>
 
@@ -94,7 +95,7 @@ class StandardHandler : virtual public StandardIf {
       case MatchErrorCode::ERROR:
         return TableOperationErrorCode::ERROR;
       default:
-        assert(0 && "invalid error code");
+        _BM_UNREACHABLE("Default switch case should not be reachable");
     }
   }
 

src/bm_sim/Makefile.am

@@ -14,6 +14,7 @@ AM_CFLAGS += $(COVERAGE_FLAGS)
 noinst_LTLIBRARIES = libbmsim.la
 
 libbmsim_la_SOURCES = \
+_assert.cpp \
 action_profile.cpp \
 actions.cpp \
 ageing.cpp \

src/bm_sim/_assert.cpp

@@ -0,0 +1,34 @@
+/* Copyright 2013-present Barefoot Networks, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * Antonin Bas ([email protected])
+ *
+ */
+
+#include <bm/bm_sim/_assert.h>
+
+#include <cstdlib>
+#include <iostream>
+
+namespace bm {
+
+void _assert(const char* expr, const char* file, int line) {
+  std::cerr << "Assertion '" << expr << "' failed, file '" << file
+            << "' line '" << line << "'.\n";
+  std::abort();
+}
+
+}  // namespace bm

src/bm_sim/action_profile.cpp

@@ -18,6 +18,7 @@
  *
  */
 
+#include <bm/bm_sim/_assert.h>
 #include <bm/bm_sim/action_profile.h>
 #include <bm/bm_sim/logger.h>
 #include <bm/bm_sim/packet.h>
@@ -243,7 +244,9 @@ ActionProfile::delete_member(mbr_hdl_t mbr) {
     } else if (index_ref_count.get(IndirectIndex::make_mbr_index(mbr)) > 0) {
       rc = MatchErrorCode::MBR_STILL_USED;
     } else {
-      assert(!mbr_handles.release_handle(mbr));
+      int error = mbr_handles.release_handle(mbr);
+      _BM_UNUSED(error);
+      assert(!error);
       num_members--;
     }
   }
@@ -338,7 +341,9 @@ ActionProfile::delete_group(grp_hdl_t grp) {
       for (auto mbr : group_info)
         index_ref_count.decrease(IndirectIndex::make_mbr_index(mbr));
 
-      assert(!grp_handles.release_handle(grp));
+      int error = grp_handles.release_handle(grp);
+      _BM_UNUSED(error);
+      assert(!error);
 
       num_groups--;
     }
@@ -448,6 +453,7 @@ ActionProfile::get_members() const {
   size_t idx = 0;
   for (const auto h : mbr_handles) {
     MatchErrorCode rc = get_member_(h, &members[idx++]);
+    _BM_UNUSED(rc);
     assert(rc == MatchErrorCode::SUCCESS);
   }
   return members;
@@ -478,6 +484,7 @@ ActionProfile::get_groups() const {
   size_t idx = 0;
   for (const auto h : grp_handles) {
     MatchErrorCode rc = get_group_(h, &groups[idx++]);
+    _BM_UNUSED(rc);
     assert(rc == MatchErrorCode::SUCCESS);
   }
   return groups;
@@ -540,14 +547,18 @@ ActionProfile::deserialize(std::istream *in, const P4Objects &objs) {
   (*in) >> num_members;
   for (size_t i = 0; i < num_members; i++) {
     mbr_hdl_t mbr_hdl; (*in) >> mbr_hdl;
-    assert(!mbr_handles.set_handle(mbr_hdl));
+    int error = mbr_handles.set_handle(mbr_hdl);
+    _BM_UNUSED(error);
+    assert(!error);
     action_entries.at(mbr_hdl).deserialize(in, objs);
   }
   index_ref_count.deserialize(in);
   (*in) >> num_groups;
   for (size_t i = 0; i < num_groups; i++) {
     grp_hdl_t grp_hdl; (*in) >> grp_hdl;
-    assert(!grp_handles.set_handle(grp_hdl));
+    int error = grp_handles.set_handle(grp_hdl);
+    _BM_UNUSED(error);
+    assert(!error);
     grp_mgr.insert_group(grp_hdl);
     grp_mgr.at(grp_hdl).deserialize(in);
     for (const auto mbr : grp_mgr.at(grp_hdl))

src/bm_sim/debugger.cpp

@@ -375,13 +375,11 @@ DebuggerNN::packet_in_(const PacketId &packet_id, int port) {
       wait_for_resume_packet_in(lock);
   }
 
-  auto it = packet_registers_map.find(packet_id);
-  assert(it == packet_registers_map.end());
+  assert(packet_registers_map.find(packet_id) == packet_registers_map.end());
   packet_registers_map.emplace_hint(packet_registers_map.end(),
                                     packet_id, PacketRegisters());
 
-  auto it2 = packet_ctr_stacks.find(packet_id);
-  assert(it2 == packet_ctr_stacks.end());
+  assert(packet_ctr_stacks.find(packet_id) == packet_ctr_stacks.end());
   packet_ctr_stacks.emplace_hint(packet_ctr_stacks.end(),
                                  packet_id, std::vector<uint32_t>());
 

src/bm_sim/dev_mgr_bmi.cpp

@@ -19,11 +19,13 @@
  */
 
 #include <bm/bm_sim/dev_mgr.h>
+#include <bm/bm_sim/logger.h>
 
-#include <string>
 #include <cassert>
+#include <cstdlib>
 #include <mutex>
 #include <map>
+#include <string>
 
 extern "C" {
 #include "BMI/bmi_port.h"
@@ -43,7 +45,10 @@ class BmiDevMgrImp : public DevMgrIface {
  public:
   BmiDevMgrImp(int device_id,
                std::shared_ptr<TransportIface> notifications_transport) {
-    assert(!bmi_port_create_mgr(&port_mgr));
+    if (bmi_port_create_mgr(&port_mgr)) {
+      Logger::get()->critical("Could not initialize BMI port manager");
+      std::exit(1);
+    }
 
     p_monitor = PortMonitorIface::make_active(device_id,
                                               notifications_transport);
@@ -86,7 +91,8 @@ class BmiDevMgrImp : public DevMgrIface {
 
   void start_() override {
     assert(port_mgr);
-    assert(!bmi_start_mgr(port_mgr));
+    if (bmi_start_mgr(port_mgr))
+      Logger::get()->critical("Could not start BMI port manager");
   }
 
   ReturnCode set_packet_handler_(const PacketHandler &handler, void *cookie)
@@ -95,7 +101,10 @@ class BmiDevMgrImp : public DevMgrIface {
     function_t * const*ptr_fun = handler.target<function_t *>();
     assert(ptr_fun);
     assert(*ptr_fun);
-    assert(!bmi_set_packet_handler(port_mgr, *ptr_fun, cookie));
+    if (bmi_set_packet_handler(port_mgr, *ptr_fun, cookie)) {
+      Logger::get()->critical("Could not set BMI packet handler");
+      return ReturnCode::ERROR;
+    }
     return ReturnCode::SUCCESS;
   }
 

src/bm_sim/lookup_structures.cpp

@@ -19,6 +19,7 @@
  *
  */
 
+#include <bm/bm_sim/_assert.h>
 #include <bm/bm_sim/lookup_structures.h>
 #include <bm/bm_sim/match_key_types.h>
 
@@ -172,7 +173,9 @@ class TernaryCache {
     if (it != cache.end()) return false;
     if (cache.size() == S) {  // cache is full
       auto evict = entries.back();
-      assert(cache.erase(evict.key) == 1);
+      int success = cache.erase(evict.key);
+      _BM_UNUSED(success);
+      assert(success == 1);
       entries.pop_back();
     }
     entries.emplace_front(handle, key_data);

src/bm_sim/match_tables.cpp

@@ -18,6 +18,7 @@
  *
  */
 
+#include <bm/bm_sim/_assert.h>
 #include <bm/bm_sim/match_tables.h>
 #include <bm/bm_sim/logger.h>
 #include <bm/bm_sim/event_logger.h>
@@ -529,6 +530,7 @@ MatchTable::get_entries() const {
   for (auto it = match_unit->handles_begin(); it != match_unit->handles_end();
        it++) {
     MatchErrorCode rc = get_entry_(*it, &entries[idx++]);
+    _BM_UNUSED(rc);
     assert(rc == MatchErrorCode::SUCCESS);
   }
 
@@ -574,6 +576,7 @@ MatchTable::set_default_entry(const ActionFn *action_fn,
                               ActionData action_data, bool is_const) {
   assert(!const_default_entry);
   auto rc = set_default_action(action_fn, std::move(action_data));
+  _BM_UNUSED(rc);
   assert(rc == MatchErrorCode::SUCCESS);
   const_default_entry = is_const;
 }
@@ -824,6 +827,7 @@ MatchTableIndirect::get_entries() const {
   for (auto it = match_unit->handles_begin(); it != match_unit->handles_end();
        it++) {
     MatchErrorCode rc = get_entry_(*it, &entries[idx++]);
+    _BM_UNUSED(rc);
     assert(rc == MatchErrorCode::SUCCESS);
   }
 
@@ -1044,6 +1048,7 @@ MatchTableIndirectWS::get_entries() const {
   for (auto it = match_unit->handles_begin(); it != match_unit->handles_end();
        it++) {
     MatchErrorCode rc = get_entry_(*it, &entries[idx++]);
+    _BM_UNUSED(rc);
     assert(rc == MatchErrorCode::SUCCESS);
   }