-
Notifications
You must be signed in to change notification settings - Fork 510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
common: Clean-up Valgrind usage in the project #5950
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5950 +/- ##
==========================================
- Coverage 70.14% 70.12% -0.03%
==========================================
Files 133 133
Lines 19568 19568
Branches 3261 3261
==========================================
- Hits 13726 13722 -4
- Misses 5842 5846 +4 |
92fb7a9
to
5775193
Compare
5775193
to
92bd788
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 12 files at r7, 2 of 2 files at r8, 3 of 3 files at r10, 1 of 2 files at r11, 1 of 2 files at r13, 6 of 6 files at r16, 1 of 3 files at r17, 2 of 2 files at r22, 1 of 1 files at r23, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @janekmi)
src/test/valgrind_check/Makefile
line 2 at r23 (raw file):
# SPDX-License-Identifier: BSD-3-Clause # Copyright 2023, Intel Corporation
Please correct the date
Suggestion:
Copyright 2024, Intel Corporation
src/test/valgrind_check/TEST0
line 3 at r23 (raw file):
#!/usr/bin/env bash # SPDX-License-Identifier: BSD-3-Clause # Copyright 2023, Intel Corporation
.
Suggestion:
Copyright 2024, Intel Corporation
src/test/valgrind_check/valgrind_check.c
line 2 at r23 (raw file):
// SPDX-License-Identifier: BSD-3-Clause /* Copyright 2023, Intel Corporation */
.
5950c56
to
80d1ff7
Compare
80d1ff7
to
8bc52d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r25, 3 of 3 files at r27, 1 of 2 files at r28, 1 of 2 files at r30, 6 of 6 files at r33, 3 of 3 files at r34, 1 of 1 files at r36, 2 of 2 files at r37, 1 of 1 files at r38, 2 of 2 files at r39, 1 of 1 files at r40, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 12 files at r7, 2 of 2 files at r25, 3 of 3 files at r27, 1 of 2 files at r28, 1 of 2 files at r30, 6 of 6 files at r33, 1 of 3 files at r34, 2 of 2 files at r39, 1 of 1 files at r40, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72)
a discussion (no related file):
I believe common: update checkout version
is effectively empty.
INSTALL.md
line 117 at r40 (raw file):
If you want to disables all Valgrind tools run:
Suggestion:
If you want to disable all Valgrind tools run:
.github/workflows/pmem_tests.yml
line 8 at r40 (raw file):
on: workflow_dispatch: pull_request:
Please drop it.
src/core/valgrind_internal.h
line 35 at r40 (raw file):
#define VG_MEMCHECK_ENABLED 0 #define VG_DRD_ENABLED 0 #endif
How did you stumble upon this issue?
Code quote:
#if VALGRIND_ENABLED
#ifndef VG_PMEMCHECK_ENABLED
#define VG_PMEMCHECK_ENABLED 1
#endif
#ifndef VG_HELGRIND_ENABLED
#define VG_HELGRIND_ENABLED 1
#endif
#ifndef VG_MEMCHECK_ENABLED
#define VG_MEMCHECK_ENABLED 1
#endif
#ifndef VG_DRD_ENABLED
#define VG_DRD_ENABLED 1
#endif
#else
#define VG_PMEMCHECK_ENABLED 0
#define VG_HELGRIND_ENABLED 0
#define VG_MEMCHECK_ENABLED 0
#define VG_DRD_ENABLED 0
#endif
src/libpmemobj/palloc.c
line 425 at r40 (raw file):
VALGRIND_REGISTER_PMEM_MAPPING(ptr, size); } #endif
What you proposed will work only assuming VG_MEMCHECK_ENABLED and VG_PMEMCHECK_ENABLED are all either turned on or off.
But it is not necessarily true. Note the code below:
#ifndef VG_PMEMCHECK_ENABLED
#define VG_PMEMCHECK_ENABLED 1
#endif
#ifndef VG_HELGRIND_ENABLED
#define VG_HELGRIND_ENABLED 1
#endif
#ifndef VG_MEMCHECK_ENABLED
#define VG_MEMCHECK_ENABLED 1
#endif
#ifndef VG_DRD_ENABLED
#define VG_DRD_ENABLED 1
#endif
means that if someone provides e.g. EXTRA_CFLAGS=-DVG_PMEMCHECK_ENABLED=0
you can have memcheck without pmemcheck. Which will result in a dangling } else
statement.
Suggestion:
#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);
}
#endif
#if VG_PMEMCHECK_ENABLED
if (On_pmemcheck) {
/*
* The sync module, responsible for implementations of
* persistent memory resident volatile variables,
* de-registers the pmemcheck pmem mapping at the time
* of initialization. This is done so that usage of
* pmem locks is not reported as an error due to
* missing flushes/stores outside of transaction. But,
* after we freed an object, we need to reestablish
* the pmem mapping, otherwise pmemchek might miss bugs
* that occur in newly allocated memory locations, that
* once were occupied by a lock/volatile variable.
*/
void *ptr = act->m.m_ops->get_user_data(&act->m);
size_t size = act->m.m_ops->get_real_size(&act->m);
VALGRIND_REGISTER_PMEM_MAPPING(ptr, size);
}
#endif
src/test/pmem2_persist_valgrind/TESTS.py
line 37 at r40 (raw file):
@t.require_valgrind_enabled('pmemcheck')
This is odd. The parent class already has pmemcheck enabled. So, what is this about?
src/test/unittest/unittest.sh
line 151 at r40 (raw file):
[ "$OBJ_VERIFY" ] || OBJ_VERIFY=$TOOLS/obj_verify/obj_verify [ "$USC_PERMISSION" ] || USC_PERMISSION=$TOOLS/usc_permission_check/usc_permission_check.static_nondebug [ "$ANONYMOUS_MMAP" ] || ANONYMOUS_MMAP=$TOOLS/anonymous_mmap/anonymous_mmap.static_nondebug
Please see examples of other test tools.
Code quote:
[ "$FIP" ] || FIP=$TOOLS/fip/fip
[ "$DDMAP" ] || DDMAP=$TOOLS/ddmap/ddmap
[ "$CMPMAP" ] || CMPMAP=$TOOLS/cmpmap/cmpmap
[ "$EXTENTS" ] || EXTENTS=$TOOLS/extents/extents
[ "$FALLOCATE_DETECT" ] || FALLOCATE_DETECT=$TOOLS/fallocate_detect/fallocate_detect.static_nondebug
[ "$OBJ_VERIFY" ] || OBJ_VERIFY=$TOOLS/obj_verify/obj_verify
[ "$USC_PERMISSION" ] || USC_PERMISSION=$TOOLS/usc_permission_check/usc_permission_check.static_nondebug
[ "$ANONYMOUS_MMAP" ] || ANONYMOUS_MMAP=$TOOLS/anonymous_mmap/anonymous_mmap.static_nondebug
src/test/unittest/unittest.sh
line 1479 at r40 (raw file):
fi #Check if Valgrind is enabeld in test build
Suggestion:
# Check if Valgrind is enabled in the test build
src/test/unittest/unittest.sh
line 1481 at r40 (raw file):
#Check if Valgrind is enabeld in test build disable_exit_on_error ../valgrind_check/valgrind_check
Shouldn't it be a test/tool?
Code quote:
../valgrind_check/valgrind_check
src/test/unittest/unittest.sh
line 1482 at r40 (raw file):
disable_exit_on_error ../valgrind_check/valgrind_check local ret1=$?
You don't need a new variable for this.
Suggestion:
ret=$?
src/test/unittest/unittest.sh
line 1486 at r40 (raw file):
if [ $ret1 -ne 0 ]; then msg=$(interactive_yellow STDOUT "SKIP:") echo -e "$UNITTEST_NAME: $msg Valgrind tool required but not enabled in the source code"
Suggestion:
Valgrind is required but the Valgrind support has been disabled at compile time
src/test/unittest/valgrind.py
line 204 at r40 (raw file):
try: out = sp.check_output('./valgrind_check/valgrind_check',
Wow. This is confusing. Different paths in different frameworks.
src/test/unittest/valgrind.py
line 204 at r40 (raw file):
try: out = sp.check_output('./valgrind_check/valgrind_check',
- You don't need it.
- You overwrite the result from the previous try-catch which is the key element of this procedure.
- This kind of bug indicates to me it is not properly tested yet.
Suggestion:
_ =
src/test/unittest/valgrind.py
line 209 at r40 (raw file):
except sp.CalledProcessError: raise futils.Skip('Valgrind tool required but not enabled' + ' in the source code')
Note: two spaces before an inline comment.
Suggestion:
raise futils.Skip(
'Valgrind is required but the Valgrind support has been disabled at compile time') # noqa: E501
src/test/valgrind_check/Makefile
line 5 at r40 (raw file):
# # src/test/pmem_movnt/Makefile -- build valgrind_check unit test
Suggestion:
src/test/valgrind_check/Makefile
src/test/valgrind_check/Makefile
line 10 at r40 (raw file):
OBJS = valgrind_check.o LIBPMEM=n
AFAIK there is no piece of code which would link the test against libpmem if not stated otherwise.
src/test/valgrind_check/README
line 7 at r40 (raw file):
This directory contains a unit test for Valgrind enabling. The program in valgrind_check.c verifies if Valgrind was enabled during build.
Personally, I hate this kind of file. Worthless word salad.
I would not mind if you dropped it by accident.
Code quote:
Persistent Memory Development Kit
This is src/test/valgrind_check/README.
This directory contains a unit test for Valgrind enabling.
The program in valgrind_check.c verifies if Valgrind was enabled during build.
src/test/valgrind_check/valgrind_check.c
line 11 at r40 (raw file):
*/ #include "unittest.h"
Not used.
608741d
to
0a49782
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 22 files reviewed, 15 unresolved discussions (waiting on @janekmi and @osalyk)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
I believe
common: update checkout version
is effectively empty.
Done.
INSTALL.md
line 117 at r40 (raw file):
If you want to disables all Valgrind tools run:
Done.
.github/workflows/pmem_tests.yml
line 8 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Please drop it.
Done.
src/libpmemobj/palloc.c
line 425 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
What you proposed will work only assuming VG_MEMCHECK_ENABLED and VG_PMEMCHECK_ENABLED are all either turned on or off.
But it is not necessarily true. Note the code below:
#ifndef VG_PMEMCHECK_ENABLED #define VG_PMEMCHECK_ENABLED 1 #endif #ifndef VG_HELGRIND_ENABLED #define VG_HELGRIND_ENABLED 1 #endif #ifndef VG_MEMCHECK_ENABLED #define VG_MEMCHECK_ENABLED 1 #endif #ifndef VG_DRD_ENABLED #define VG_DRD_ENABLED 1 #endifmeans that if someone provides e.g.
EXTRA_CFLAGS=-DVG_PMEMCHECK_ENABLED=0
you can have memcheck without pmemcheck. Which will result in a dangling} else
statement.
Done.
src/test/unittest/unittest.sh
line 1479 at r40 (raw file):
fi #Check if Valgrind is enabeld in test build
Done.
src/test/unittest/unittest.sh
line 1481 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Shouldn't it be a test/tool?
A separate PR will be.
src/test/unittest/unittest.sh
line 1482 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
You don't need a new variable for this.
Done.
src/test/unittest/unittest.sh
line 1486 at r40 (raw file):
if [ $ret1 -ne 0 ]; then msg=$(interactive_yellow STDOUT "SKIP:") echo -e "$UNITTEST_NAME: $msg Valgrind tool required but not enabled in the source code"
Done.
src/test/unittest/valgrind.py
line 204 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Wow. This is confusing. Different paths in different frameworks.
Yes.
RUNTEST.py is executed under src/test but RUNTEST.sh goes into subfolders
src/test/unittest/valgrind.py
line 204 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
- You don't need it.
- You overwrite the result from the previous try-catch which is the key element of this procedure.
- This kind of bug indicates to me it is not properly tested yet.
Done.
src/test/unittest/valgrind.py
line 209 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Note: two spaces before an inline comment.
Done.
src/test/valgrind_check/Makefile
line 5 at r40 (raw file):
# # src/test/pmem_movnt/Makefile -- build valgrind_check unit test
Done.
src/test/valgrind_check/Makefile
line 10 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
AFAIK there is no piece of code which would link the test against libpmem if not stated otherwise.
Done.
src/test/valgrind_check/valgrind_check.c
line 11 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Not used.
Done.
src/test/pmem2_persist_valgrind/TESTS.py
line 37 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
This is odd. The parent class already has pmemcheck enabled. So, what is this about?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 22 files reviewed, 15 unresolved discussions (waiting on @janekmi and @osalyk)
src/test/valgrind_check/README
line 7 at r40 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Personally, I hate this kind of file. Worthless word salad.
I would not mind if you dropped it by accident.
A separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 18 files at r41, 2 of 2 files at r42, 3 of 3 files at r44, 1 of 2 files at r45, 4 of 7 files at r49, 3 of 6 files at r50, 1 of 2 files at r52, 1 of 2 files at r54, 2 of 2 files at r55, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
src/libpmemobj/palloc.c
line 408 at r55 (raw file):
#if VG_PMEMCHECK_ENABLED else #endif /* VG_PMEMCHECK_ENABLED */
That's odd. Can we get rid of it?
Code quote:
#if VG_PMEMCHECK_ENABLED
else
#endif /* VG_PMEMCHECK_ENABLED */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 17 of 34 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
.github/workflows/pmem_tests.yml
line 20 at r117 (raw file):
# in the dedicated workflows below. force_enable: '["none"]' valgrind: 0
Done.
.github/workflows/pmem_tests.yml
line 120 at r117 (raw file):
# Exclude all Valgrind tests but keep Valgrind tools built-in. force_enable: '["none"]' valgrind: 1
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 17 of 34 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
src/test/valgrind_check/valgrind_check.c
line 24 at r117 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
No
It will be addressed in a separate PR
ca73eef
to
bf1a77a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 17 of 34 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
ChangeLog
line 10 at r117 (raw file):
Previously, janekmi (Jan Michalski) wrote…
I don't get it.
- Is it really a new thing?
- What checks do you mean?
Done.
bf1a77a
to
efa1085
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 16 of 34 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
src/libpmemobj/palloc.c
line 425 at r104 (raw file):
VALGRIND_REGISTER_PMEM_MAPPING(ptr, size); } #endif /* VG_MEMCHECK_ENABLED */
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 17 files at r118, 2 of 6 files at r120, 2 of 3 files at r121, 2 of 14 files at r122, 6 of 6 files at r123, 3 of 3 files at r124, 3 of 3 files at r125, all commit messages.
Reviewable status: 33 of 34 files reviewed, 5 unresolved discussions (waiting on @grom72, @janekmi, and @osalyk)
.github/workflows/pmem_tests.yml
line 120 at r117 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Done.
Not yet.
src/Makefile.inc
line 112 at r87 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Can you explain why?
It was just meant to stop progressing too quickly towards dropping the Valgrind support by default. I believe we are already the same page on this.
efa1085
to
bd347c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r91, 30 of 30 files at r105, 4 of 7 files at r106, 1 of 3 files at r107, 2 of 5 files at r108, 5 of 18 files at r109, 2 of 17 files at r118, 14 of 14 files at r122.
Reviewable status: 31 of 34 files reviewed, 5 unresolved discussions (waiting on @janekmi and @osalyk)
.github/workflows/pmem_tests.yml
line 120 at r117 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Not yet.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r127, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72 and @osalyk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 25 of 30 files at r105, 2 of 3 files at r107, 1 of 2 files at r117, 1 of 17 files at r118, 1 of 6 files at r123, 3 of 3 files at r127, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @janekmi)
b66b848
to
5f00672
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 15 of 16 files at r129, 6 of 6 files at r130, 3 of 3 files at r131, 2 of 3 files at r132, 3 of 3 files at r133, all commit messages.
Reviewable status: 33 of 34 files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Disable all Valgrind tools when Valgrind is not enabled Use VALGRIND=0 env. var. to disable Valgrind in compilation Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Test is skipped if any Valgrind tool is required and Valgrind was not enabled during build (VALGRIND=0) Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
disable valgrind in builds that do not need them Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r91, 1 of 16 files at r129.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
It still waits for the final validation. Please confirm when done.
https://github.com/pmem/pmdk/actions/runs/8365883478/job/22904793478
https://github.com/pmem/pmdk/actions/runs/8366004352
https://github.com/pmem/pmdk/actions/runs/8366006329/job/22905198088 - not related
https://github.com/pmem/pmdk/actions/runs/8365883478/job/22904788695#step:3:5 - w/o NDCTL fix works
DONE
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
5f00672
to
8188ab6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r115, 16 of 16 files at r129, 6 of 6 files at r130, 3 of 3 files at r131, 2 of 3 files at r132, 3 of 3 files at r133, 16 of 16 files at r135, 6 of 6 files at r136, 3 of 3 files at r137, 1 of 3 files at r138, 1 of 1 files at r139, 1 of 1 files at r140, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 16 files at r135, 6 of 6 files at r136, 3 of 3 files at r137, 1 of 3 files at r138, 1 of 1 files at r139, 1 of 1 files at r140, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)
common: disable On_pmemcheck access outside the Valgrind context
This change is