Merge pull request pmem#5950 from grom72/obsolete-on_pmemcheck

janekmi · web-flow · commit d4bd6f2d73c4 · 2024-03-25T21:47:33.000Z
common: Clean-up Valgrind usage in the project
diff --git a/.github/actions/pmem_test_prepare/action.yml b/.github/actions/pmem_test_prepare/action.yml
@@ -9,6 +9,10 @@ inputs:
     description: Build with the fault injection capability
     required: false
     default: '0'
+  valgrind:
+    description: Build with Valgrind support
+    required: true
+
 runs:
   using: composite
   steps:
@@ -30,6 +34,9 @@ runs:
     - env:
         FAULT_INJECTION: ${{ inputs.fault_injection }}
         NDCTL_ENABLE: ${{ inputs.ndctl_enable }}
+        PMEMOBJ_IGNORE_DIRTY_SHUTDOWN: ${{ inputs.ndctl_enable == 'n' && 'y' || 'n' }}
+        PMEMOBJ_IGNORE_BAD_BLOCKS: ${{ inputs.ndctl_enable == 'n' && 'y' || 'n' }}
+        VALGRIND: ${{ inputs.valgrind }}
       run: |
         echo '::group::Build'
         $WORKDIR/build-pmdk.sh
diff --git a/.github/workflows/pmem_test_matrix.yml b/.github/workflows/pmem_test_matrix.yml
@@ -9,6 +9,9 @@ on:
       force_enable:
         required: true
         type: string
+      valgrind:
+        required: true
+        type: string
       timeout_minutes:
         required: false
         type: number
@@ -28,11 +31,14 @@ jobs:
         os: [rhel, opensuse]
         build: [debug, nondebug]
 
+
     steps:
       - uses: actions/checkout@v4
 
       - name: Test prepare
         uses: ./.github/actions/pmem_test_prepare
+        with:
+          valgrind: ${{ inputs.valgrind }}
 
       - name: Test run
         uses: ./.github/actions/pmem_test_run
diff --git a/.github/workflows/pmem_tests.yml b/.github/workflows/pmem_tests.yml
@@ -12,11 +12,16 @@ on:
 jobs:
   # Test the default build with the basic test suite.
   Basic:
+    strategy:
+      matrix:
+        VALGRIND: [0, 1]
+    name: Basic ${{ matrix.VALGRIND == 0  && 'w/o Valgrind' || 'w/ Valgrind' }}
     uses: ./.github/workflows/pmem_test_matrix.yml
     with:
       # Exclude all Valgrind tests. All tests employing Valgrind tooling are run
       # in the dedicated workflows below.
       force_enable: '["none"]'
+      valgrind: ${{ matrix.VALGRIND }}
 
 
   # Test the default build with force-enabled Valgrind tooling for (persistent)
@@ -25,6 +30,7 @@ jobs:
     uses: ./.github/workflows/pmem_test_matrix.yml
     with:
       force_enable: '["pmemcheck", "memcheck"]'
+      valgrind: 1
 
 
   # Test the default build with force-enabled Valgrind tooling for thread error
@@ -33,6 +39,7 @@ jobs:
     uses: ./.github/workflows/pmem_test_matrix.yml
     with:
       force_enable: '["drd", "helgrind"]'
+      valgrind: 1
       # 9h = 7h20m (the longest workflow execution time) + ~20% leeway.
       timeout_minutes: 540
 
@@ -52,6 +59,8 @@ jobs:
 
       - name: Test prepare
         uses: ./.github/actions/pmem_test_prepare
+        with:
+          valgrid: 1
 
       - name: Test run
         uses: ./.github/actions/pmem_test_run
@@ -73,6 +82,7 @@ jobs:
         uses: ./.github/actions/pmem_test_prepare
         with:
           fault_injection: '1'
+          valgrind: 1
 
       - name: Test run
         uses: ./.github/actions/pmem_test_run
@@ -84,6 +94,8 @@ jobs:
   # By default, PMDK is built with NDCTL in order to provide RAS features.
   # This build is only viable as long as DAOS builds PMDK with NDCTL_ENABLE=n
   # https://github.com/daos-stack/pmdk/pull/12
+  # It should be removed as soon as following PR is merged
+  # https://github.com/daos-stack/pmdk/pull/35
   ndctl_enable_n:
     name: Without ndctl
     if: github.repository == 'pmem/pmdk'
@@ -96,6 +108,7 @@ jobs:
         uses: ./.github/actions/pmem_test_prepare
         with:
           ndctl_enable: n
+          valgrind: 0
 
       - name: Test run
         uses: ./.github/actions/pmem_test_run
diff --git a/.github/workflows/scan_codeql.yml b/.github/workflows/scan_codeql.yml
@@ -41,7 +41,7 @@ jobs:
         languages: cpp, python
 
     - name: Build PMDK
-      run: make EXTRA_CFLAGS=-DUSE_VALGRIND test -j$(nproc)
+      run: make test -j$(nproc)
 
     - name: CodeQL scan
       uses: github/codeql-action/analyze@v2
diff --git a/ChangeLog b/ChangeLog
@@ -17,7 +17,6 @@ Mon Dec 4 2023 Oksana Sałyk <oksana.salyk@intel.com>
 	- Reduces libpmemobj's stack usage below the 11kB threshold.
 	- Fixing minor Coverity issues
 	- Add a new toolset for stack usage analysis (https://github.com/pmem/pmdk/tree/master/utils/call_stacks_analysis)
-	
 
 Tue Aug 8 2023 Oksana Sałyk <oksana.salyk@intel.com>
 
diff --git a/INSTALL.md b/INSTALL.md
@@ -108,15 +108,12 @@ Both building and installation scripts are very flexible. To see additional opti
 ### Memory Management Tools
 
 The PMDK libraries support standard Valgrind DRD, Helgrind and Memcheck, as well as a PM-aware version of [Valgrind](https://github.com/pmem/valgrind).
-By default, support for all tools is enabled. If you wish to disable it, supply the compiler with `VG_\<TOOL\>_ENABLED` flag set to 0, for example:
+By default, support for all tools is enabled. If you wish to disable it, supply the compiler with `VALGRIND` flag set to 0:
 
 ```sh
-make EXTRA_CFLAGS=-DVG_MEMCHECK_ENABLED=0
+make VALGRIND=0
 ```
 
-`VALGRIND_ENABLED` flag, when set to 0, disables all Valgrind tools
-(drd, helgrind, memcheck and pmemcheck).
-
 The `SANITIZE` flag allows the libraries to be tested with various
 sanitizers. For example, to test the libraries with AddressSanitizer
 and UndefinedBehaviorSanitizer, run:
diff --git a/src/benchmarks/Makefile b/src/benchmarks/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright 2014-2023, Intel Corporation
+# Copyright 2014-2024, Intel Corporation
 
 #
 # src/benchmarks/Makefile -- build all benchmarks
@@ -109,6 +109,11 @@ LDFLAGS += $(GCOV_LDFLAGS)
 LIBS += $(GCOV_LIBS)
 endif
 
+ifeq ($(VALGRIND),0)
+CFLAGS += -DVALGRIND_ENABLED=0
+CXXFLAGS += -DVALGRIND_ENABLED=0
+endif
+
 ifneq ($(SANITIZE),)
 CXXFLAGS += -fsanitize=$(SANITIZE)
 LDFLAGS += -fsanitize=$(SANITIZE)
diff --git a/src/core/valgrind_internal.h b/src/core/valgrind_internal.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: BSD-3-Clause */
-/* Copyright 2015-2023, Intel Corporation */
+/* Copyright 2015-2024, Intel Corporation */
 
 /*
  * valgrind_internal.h -- internal definitions for valgrind macros
@@ -19,6 +19,11 @@
 #define VG_HELGRIND_ENABLED 1
 #define VG_MEMCHECK_ENABLED 1
 #define VG_DRD_ENABLED 1
+#else
+#define VG_PMEMCHECK_ENABLED 0
+#define VG_HELGRIND_ENABLED 0
+#define VG_MEMCHECK_ENABLED 0
+#define VG_DRD_ENABLED 0
 #endif
 
 #if VG_PMEMCHECK_ENABLED || VG_HELGRIND_ENABLED || VG_MEMCHECK_ENABLED || \
diff --git a/src/examples/Makefile.inc b/src/examples/Makefile.inc
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright 2015-2020, Intel Corporation
+# Copyright 2015-2024, Intel Corporation
 
 #
 # examples/Makefile.inc -- build the Persistent Memory Development Kit examples
@@ -23,6 +23,10 @@ CFLAGS += -fsanitize=$(SANITIZE)
 CXXFLAGS += -fsanitize=$(SANITIZE)
 LDFLAGS += -fsanitize=$(SANITIZE)
 endif
+ifeq ($(VALGRIND),0)
+CFLAGS += -DVALGRIND_ENABLED=0
+CXXFLAGS += -DVALGRIND_ENABLED=0
+endif
 INCS = -I$(INCDIR) -I. -I$(TOP_SRC)/examples $(OS_INCS)
 LIBS += $(OS_LIBS) $(LIBUUID)
 
diff --git a/src/libpmem2/x86_64/memcpy/memcpy_avx.h b/src/libpmem2/x86_64/memcpy/memcpy_avx.h
@@ -87,10 +87,13 @@ memmove_small_avx(char *dest, const char *src, size_t len, flush_fn flush)
 	 * path) in the optimized version.
 	 * libc's memcpy also does that, so we can't use it here.
 	 */
+#if VG_PMEMCHECK_ENABLED
 	if (On_pmemcheck) {
 		memmove_nodrain_generic(dest, src, len, PMEM2_F_MEM_NOFLUSH,
 				NULL, NULL);
-	} else {
+	} else
+#endif
+	{
 		memmove_small_avx_noflush(dest, src, len);
 	}
 
diff --git a/src/libpmem2/x86_64/memcpy/memcpy_sse2.h b/src/libpmem2/x86_64/memcpy/memcpy_sse2.h
@@ -103,10 +103,13 @@ memmove_small_sse2(char *dest, const char *src, size_t len, flush_fn flush)
 	 * path) in the optimized version.
 	 * libc's memcpy also does that, so we can't use it here.
 	 */
+#if VG_PMEMCHECK_ENABLED
 	if (On_pmemcheck) {
 		memmove_nodrain_generic(dest, src, len, PMEM2_F_MEM_NOFLUSH,
 				NULL, NULL);
-	} else {
+	} else
+#endif
+	{
 		memmove_small_sse2_noflush(dest, src, len);
 	}
 
diff --git a/src/libpmem2/x86_64/memset/memset_avx.h b/src/libpmem2/x86_64/memset/memset_avx.h
@@ -84,11 +84,15 @@ memset_small_avx(char *dest, __m256i ymm, size_t len, flush_fn flush)
 	 * path) in the optimized version.
 	 * libc's memset also does that, so we can't use it here.
 	 */
+#if VG_PMEMCHECK_ENABLED
 	if (On_pmemcheck) {
 		memset_nodrain_generic(dest, (uint8_t)m256_get2b(ymm),
 						len, PMEM2_F_MEM_NOFLUSH, NULL,
 						NULL);
-	} else {
+	}
+	else
+#endif
+	{
 		memset_small_avx_noflush(dest, ymm, len);
 	}
 
diff --git a/src/libpmem2/x86_64/memset/memset_sse2.h b/src/libpmem2/x86_64/memset/memset_sse2.h
@@ -91,10 +91,13 @@ memset_small_sse2(char *dest, __m128i xmm, size_t len, flush_fn flush)
 	 * path) in the optimized version.
 	 * libc's memset also does that, so we can't use it here.
 	 */
+#if VG_PMEMCHECK_ENABLED
 	if (On_pmemcheck) {
 		memset_nodrain_generic(dest, (uint8_t)_mm_cvtsi128_si32(xmm),
 				len, PMEM2_F_MEM_NOFLUSH, NULL, NULL);
-	} else {
+	} else
+#endif
+	{
 		memset_small_sse2_noflush(dest, xmm, len);
 	}
 
diff --git a/src/libpmemobj/heap.c b/src/libpmemobj/heap.c
@@ -1240,6 +1240,7 @@ heap_get_bestfit_block(struct palloc_heap *heap, struct bucket *b,
 	return 0;
 }
 
+#if VG_MEMCHECK_ENABLED
 /*
  * heap_end -- returns first address after heap
  */
@@ -1252,6 +1253,7 @@ heap_end(struct palloc_heap *h)
 
 	return &last_zone->chunks[last_zone->header.size_idx];
 }
+#endif /* VG_MEMCHECK_ENABLED */
 
 /*
  * heap_arena_create -- create a new arena, push it to the vector
diff --git a/src/libpmemobj/heap.h b/src/libpmemobj/heap.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: BSD-3-Clause */
-/* Copyright 2015-2022, Intel Corporation */
+/* Copyright 2015-2024, Intel Corporation */
 
 /*
  * heap.h -- internal definitions for heap
@@ -83,7 +83,9 @@ void heap_foreach_object(struct palloc_heap *heap, object_callback cb,
 
 struct alloc_class_collection *heap_alloc_classes(struct palloc_heap *heap);
 
+#if VG_MEMCHECK_ENABLED
 void *heap_end(struct palloc_heap *heap);
+#endif /* VG_MEMCHECK_ENABLED */
 
 unsigned heap_get_narenas_total(struct palloc_heap *heap);
 
diff --git a/src/libpmemobj/obj.c b/src/libpmemobj/obj.c
@@ -956,7 +956,7 @@ obj_runtime_init(PMEMobjpool *pop, int rdonly, int boot, unsigned nlanes)
 			VALGRIND_DO_MAKE_MEM_NOACCESS(end,
 				(char *)pop + pop->set->poolsize - (char *)end);
 		}
-#endif
+#endif /* VG_MEMCHECK_ENABLED */
 
 		obj_pool_init();
 
diff --git a/src/libpmemobj/palloc.c b/src/libpmemobj/palloc.c
@@ -398,10 +398,14 @@ palloc_heap_action_on_process(struct palloc_heap *heap,
 				act->m.m_ops->get_real_size(&act->m));
 		}
 	} else if (act->new_state == MEMBLOCK_FREE) {
+#if VG_MEMCHECK_ENABLED
 		if (On_memcheck) {
 			void *ptr = act->m.m_ops->get_user_data(&act->m);
 			VALGRIND_DO_MEMPOOL_FREE(heap->layout, ptr);
-		} else if (On_pmemcheck) {
+		}
+#endif /* VG_MEMCHECK_ENABLED */
+#if VG_PMEMCHECK_ENABLED
+		if (On_pmemcheck) {
 			/*
 			 * The sync module, responsible for implementations of
 			 * persistent memory resident volatile variables,
@@ -418,7 +422,7 @@ palloc_heap_action_on_process(struct palloc_heap *heap,
 			size_t size = act->m.m_ops->get_real_size(&act->m);
 			VALGRIND_REGISTER_PMEM_MAPPING(ptr, size);
 		}
-
+#endif /* VG_PMEMCHECK_ENABLED */
 		STATS_SUB(heap->stats, persistent, heap_curr_allocated,
 			act->m.m_ops->get_real_size(&act->m));
 		if (act->m.type == MEMORY_BLOCK_RUN) {
@@ -1275,6 +1279,7 @@ palloc_init(void *heap_start, uint64_t heap_size, uint64_t *sizep,
 	return heap_init(heap_start, heap_size, sizep, p_ops);
 }
 
+#if VG_MEMCHECK_ENABLED
 /*
  * palloc_heap_end -- returns first address after heap
  */
@@ -1283,6 +1288,7 @@ palloc_heap_end(struct palloc_heap *h)
 {
 	return heap_end(h);
 }
+#endif /* VG_MEMCHECK_ENABLED */
 
 /*
  * palloc_heap_check -- verifies heap state
diff --git a/src/libpmemobj/palloc.h b/src/libpmemobj/palloc.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: BSD-3-Clause */
-/* Copyright 2015-2022, Intel Corporation */
+/* Copyright 2015-2024, Intel Corporation */
 
 /*
  * palloc.h -- internal definitions for persistent allocator
@@ -89,7 +89,9 @@ int palloc_buckets_init(struct palloc_heap *heap);
 
 int palloc_init(void *heap_start, uint64_t heap_size, uint64_t *sizep,
 	struct pmem_ops *p_ops);
+#if VG_MEMCHECK_ENABLED
 void *palloc_heap_end(struct palloc_heap *h);
+#endif /* VG_MEMCHECK_ENABLED */
 int palloc_heap_check(void *heap_start, uint64_t heap_size);
 void palloc_heap_cleanup(struct palloc_heap *heap);
 size_t palloc_heap(void *heap_start);
diff --git a/src/test/Makefile b/src/test/Makefile
@@ -150,7 +150,8 @@ OTHER_TESTS = \
 	util_uuid_generate\
 	util_vec\
 	util_vecq\
-	log_errno
+	log_errno\
+	valgrind_check
 
 ifeq ($(ARCH), x86_64)
 OTHER_TESTS += \
diff --git a/src/test/RUNTESTS.py b/src/test/RUNTESTS.py
@@ -121,7 +121,9 @@ def run_tests(self):
                             continue
 
                     except futils.Skip as s:
-                        self.msg.print('{}: SKIP: {}'.format(t, s))
+                        self.msg.print('{}: {}SKIP{}: {}'.
+                                       format(t, futils.Color.YELLOW,
+                                              futils.Color.END, s))
 
                     except futils.Fail as f:
                         self._test_failed(t, c, f)
@@ -130,7 +132,9 @@ def run_tests(self):
                         self._test_passed(t)
 
             except futils.Skip as s:
-                self.msg.print('{}: SKIP: {}'.format(tc, s))
+                self.msg.print('{}: {}SKIP{}: {}'.
+                               format(tc, futils.Color.YELLOW,
+                                      futils.Color.END, s))
             except futils.Fail as f:
                 self._test_failed(tc, c, f)
                 ret = 1
diff --git a/src/test/unittest/Makefile b/src/test/unittest/Makefile
diff --git a/src/test/unittest/unittest.sh b/src/test/unittest/unittest.sh
diff --git a/src/test/unittest/valgrind.py b/src/test/unittest/valgrind.py
diff --git a/src/test/valgrind_check/.gitignore b/src/test/valgrind_check/.gitignore
diff --git a/src/test/valgrind_check/Makefile b/src/test/valgrind_check/Makefile
diff --git a/src/test/valgrind_check/README b/src/test/valgrind_check/README
diff --git a/src/test/valgrind_check/TEST0 b/src/test/valgrind_check/TEST0
diff --git a/src/test/valgrind_check/valgrind_check.c b/src/test/valgrind_check/valgrind_check.c
diff --git a/utils/call_stacks_analysis/make_extra.py b/utils/call_stacks_analysis/make_extra.py
diff --git a/utils/gha-runners/build-pmdk.sh b/utils/gha-runners/build-pmdk.sh
