From 0639f88249b14104b07651a362a004fd72735dd5 Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Mon, 21 Oct 2024 23:31:11 -0700 Subject: [PATCH 1/4] Make sure p4orch tests always run, not just when gcov is enabled Signed-off-by: Saikrishna Arcot --- orchagent/Makefile.am | 2 -- 1 file changed, 2 deletions(-) diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index 44714f62a1..5224751f73 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -8,9 +8,7 @@ INCLUDES = -I $(top_srcdir)/lib \ -I pbh \ -I nhg -if GCOV_ENABLED SUBDIRS = p4orch/tests -endif CFLAGS_SAI = -I /usr/include/sai From f014d659624f246a692e45fb38b9cbacbeed92cd Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Mon, 21 Oct 2024 23:32:45 -0700 Subject: [PATCH 2/4] Update JSON fields in p4orch for latest SAI Signed-off-by: Saikrishna Arcot --- orchagent/p4orch/tests/wcmp_manager_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/orchagent/p4orch/tests/wcmp_manager_test.cpp b/orchagent/p4orch/tests/wcmp_manager_test.cpp index 088264bba4..9e0c575cfd 100644 --- a/orchagent/p4orch/tests/wcmp_manager_test.cpp +++ b/orchagent/p4orch/tests/wcmp_manager_test.cpp @@ -2291,7 +2291,7 @@ TEST_F(WcmpManagerTest, WatchportStateChangetoOperDownSucceeds) // Verify that the next hop member associated with the port is pruned. std::string op = "port_state_change"; std::string data = "[{\"port_id\":\"oid:0x56789abcdff\",\"port_state\":\"SAI_PORT_OPER_" - "STATUS_DOWN\"}]"; + "STATUS_DOWN\",\"port_error_status\":\"0\"}]"; EXPECT_CALL(mock_sai_next_hop_group_, remove_next_hop_group_member(Eq(kWcmpGroupMemberOid1))) .WillOnce(Return(SAI_STATUS_SUCCESS)); HandlePortStatusChangeNotification(op, data); @@ -2314,7 +2314,7 @@ TEST_F(WcmpManagerTest, WatchportStateChangeToOperUpSucceeds) // restored. std::string op = "port_state_change"; std::string data = "[{\"port_id\":\"oid:0x112233\",\"port_state\":\"SAI_PORT_OPER_" - "STATUS_UP\"}]"; + "STATUS_UP\",\"port_error_status\":\"0\"}]"; EXPECT_CALL(mock_sai_next_hop_group_, create_next_hop_group_member(_, Eq(gSwitchId), Eq(3), Truly(std::bind(MatchSaiNextHopGroupMemberAttribute, kNexthopOid1, 2, @@ -2339,7 +2339,7 @@ TEST_F(WcmpManagerTest, WatchportStateChangeFromOperUnknownToDownPrunesMemberOnl // Verify that the pruned next hop member is not pruned again. std::string op = "port_state_change"; std::string data = "[{\"port_id\":\"oid:0x56789abcfff\",\"port_state\":\"SAI_PORT_OPER_" - "STATUS_DOWN\"}]"; + "STATUS_DOWN\",\"port_error_status\":\"0\"}]"; HandlePortStatusChangeNotification(op, data); EXPECT_TRUE(VerifyWcmpGroupMemberInPortMap(app_db_entry.wcmp_group_members[0], true, 1)); EXPECT_TRUE(app_db_entry.wcmp_group_members[0]->pruned); From e7bb63a6ffd9644f74da0842fbb781ca45353517 Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Tue, 22 Oct 2024 09:24:06 -0700 Subject: [PATCH 3/4] Disable the disabled-optimization for 2 ACL Manager tests in p4orch 2 test functions in acl_manager_test.cpp for p4orch are near 400 lines, with macro expansion almost certainly adding a number of lines. On armhf and arm64, GCC is complaining that it ran out of memory to do optimizations here (specifically, keep track of const and copies). ``` error: const/copy propagation disabled: 9401 basic blocks and 114813 registers; increase '--param max-gcse-memory' above 134923152 [-Werror=disabled-optimization] ``` For now, turn the error into a warning for these two functions. Ideally, these two test cases should be split up. Signed-off-by: Saikrishna Arcot --- orchagent/p4orch/tests/acl_manager_test.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/orchagent/p4orch/tests/acl_manager_test.cpp b/orchagent/p4orch/tests/acl_manager_test.cpp index 13856c07ea..18128c2705 100644 --- a/orchagent/p4orch/tests/acl_manager_test.cpp +++ b/orchagent/p4orch/tests/acl_manager_test.cpp @@ -2849,6 +2849,8 @@ TEST_F(AclManagerTest, AclRuleWithColorPacketActionsButNoRateLimit) acl_rule->action_fvs[SAI_ACL_ENTRY_ATTR_ACTION_SET_USER_TRAP_ID].aclaction.parameter.oid); } +#pragma GCC diagnostic warning "-Wdisabled-optimization" + TEST_F(AclManagerTest, AclRuleWithValidAction) { ASSERT_NO_FATAL_FAILURE(AddDefaultIngressTable()); @@ -3201,6 +3203,8 @@ TEST_F(AclManagerTest, AclRuleWithValidAction) EXPECT_EQ(nullptr, GetAclRule(kAclIngressTableName, acl_rule_key)); } +#pragma GCC diagnostic pop + TEST_F(AclManagerTest, AclRuleWithVrfAction) { ASSERT_NO_FATAL_FAILURE(AddDefaultIngressTable()); @@ -3410,6 +3414,8 @@ TEST_F(AclManagerTest, AclRuleWithIpTypeBitEncoding) ASSERT_EQ(nullptr, acl_rule); } +#pragma GCC diagnostic warning "-Wdisabled-optimization" + TEST_F(AclManagerTest, UpdateAclRuleWithActionMeterChange) { ASSERT_NO_FATAL_FAILURE(AddDefaultIngressTable()); @@ -3834,6 +3840,8 @@ TEST_F(AclManagerTest, UpdateAclRuleWithActionMeterChange) acl_rule->action_fvs[SAI_ACL_ENTRY_ATTR_ACTION_SET_USER_TRAP_ID].aclaction.parameter.oid); } +#pragma GCC diagnostic pop + TEST_F(AclManagerTest, UpdateAclRuleWithVrfActionChange) { ASSERT_NO_FATAL_FAILURE(AddDefaultIngressTable()); From 2981db7dd09989f6def61bb83af6c01b2949a28a Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Tue, 22 Oct 2024 12:21:27 -0700 Subject: [PATCH 4/4] Don't compile separate versions of p4orch_tests for ASAN/TSAN/UBSAN Instead of manually compiling separate versions of p4orch_tests for ASAN/TSAN/UBSAN, have it instead be controlled by the top-level --asan-enabled configuration flag. This matches the behavior for all other tests in this repo. This fixes an issue where armhf TSAN is not available, and arm64 TSAN is not supported (it results in an error on startup). Signed-off-by: Saikrishna Arcot --- orchagent/p4orch/tests/Makefile.am | 33 +++++------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/orchagent/p4orch/tests/Makefile.am b/orchagent/p4orch/tests/Makefile.am index 2c7301311f..74e3d7aa5c 100644 --- a/orchagent/p4orch/tests/Makefile.am +++ b/orchagent/p4orch/tests/Makefile.am @@ -4,9 +4,9 @@ INCLUDES = -I $(top_srcdir) -I $(ORCHAGENT_DIR) -I $(P4ORCH_DIR) -I $(top_srcdir CFLAGS_SAI = -I /usr/include/sai -TESTS = p4orch_tests p4orch_tests_asan p4orch_tests_tsan p4orch_tests_usan +TESTS = p4orch_tests -noinst_PROGRAMS = p4orch_tests p4orch_tests_asan p4orch_tests_tsan p4orch_tests_usan +noinst_PROGRAMS = p4orch_tests if DEBUG DBGFLAGS = -ggdb -DDEBUG @@ -16,11 +16,6 @@ endif CFLAGS_GTEST = LDADD_GTEST = -lgtest -lgtest_main -lgmock -lgmock_main -CFLAGS_COVERAGE = --coverage -fprofile-arcs -ftest-coverage -LDADD_COVERAGE = -lgcov -CFLAGS_ASAN = -fsanitize=address -CFLAGS_TSAN = -fsanitize=thread -CFLAGS_USAN = -fsanitize=undefined p4orch_tests_SOURCES = $(ORCHAGENT_DIR)/orch.cpp \ $(ORCHAGENT_DIR)/vrforch.cpp \ @@ -83,24 +78,6 @@ p4orch_tests_SOURCES = $(ORCHAGENT_DIR)/orch.cpp \ mock_sai_switch.cpp \ mock_sai_udf.cpp -p4orch_tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_COVERAGE) $(CFLAGS_SAI) -p4orch_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_COVERAGE) $(CFLAGS_SAI) -p4orch_tests_LDADD = $(LDADD_GTEST) $(LDADD_COVERAGE) -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata -lzmq - -p4orch_tests_asan_SOURCES = $(p4orch_tests_SOURCES) -p4orch_tests_asan_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_ASAN) $(CFLAGS_SAI) -p4orch_tests_asan_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_ASAN) $(CFLAGS_SAI) -p4orch_tests_asan_LDFLAGS = $(CFLAGS_ASAN) -p4orch_tests_asan_LDADD = $(LDADD_GTEST) -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata -lzmq - -p4orch_tests_tsan_SOURCES = $(p4orch_tests_SOURCES) -p4orch_tests_tsan_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_TSAN) $(CFLAGS_SAI) -p4orch_tests_tsan_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_TSAN) $(CFLAGS_SAI) -p4orch_tests_tsan_LDFLAGS = $(CFLAGS_TSAN) -p4orch_tests_tsan_LDADD = $(LDADD_GTEST) -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata -lzmq - -p4orch_tests_usan_SOURCES = $(p4orch_tests_SOURCES) -p4orch_tests_usan_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_USAN) $(CFLAGS_SAI) -p4orch_tests_usan_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_USAN) $(CFLAGS_SAI) -p4orch_tests_usan_LDFLAGS = $(CFLAGS_USAN) -p4orch_tests_usan_LDADD = $(LDADD_GTEST) -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata -lzmq +p4orch_tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(CFLAGS_ASAN) +p4orch_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(CFLAGS_ASAN) +p4orch_tests_LDADD = $(LDADD_GTEST) $(LDFLAGS_ASAN) -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata -lzmq