pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting #5605

lplewa · 2023-04-25T10:56:51Z

This change is

wlemkows · 2023-04-25T11:07:52Z

src/libpmem2/map_posix.c:1: error: wrong copyright date: (is: 2019-2021, should be: 2019-2023)
src/test/unittest/ut_file.c:1: error: wrong copyright date: (is: 2014-2020, should be: 2014-2023)

ldorau

@lplewa It fixes #5597 and not #5598

ldorau

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lplewa)

-- commits line 4 at r1:
#5597

Code quote:

#5598

grom72

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lplewa)

src/libpmem2/map_posix.c line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2019-2021, Intel Corporation */

Suggestion:

src/test/unittest/ut_file.c line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2014-2020, Intel Corporation */

Suggestion:

2014-2023

grom72

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lplewa)

grom72

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lplewa)

grom72

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lplewa)

codecov · 2023-04-27T12:05:21Z

Codecov Report

Merging #5605 (8dcb79a) into stable-1.13 (59281bb) will increase coverage by 1.57%.
The diff coverage is n/a.

❗ Current head 8dcb79a differs from pull request most recent head aea64b2. Consider uploading reports for the commit aea64b2 to get more accurate results

@@               Coverage Diff               @@
##           stable-1.13    #5605      +/-   ##
===============================================
+ Coverage        72.68%   74.25%   +1.57%     
===============================================
  Files              145      145              
  Lines            22634    22131     -503     
  Branches          3771     3704      -67     
===============================================
- Hits             16451    16433      -18     
+ Misses            6183     5698     -485

ldorau

@lplewa please rebase to stable-1.13

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lplewa)

janekmi

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lplewa)

a discussion (no related file):
Maybe it is just me, but after applying these changes I still have one remaining problem:

$ ./RUNTESTS.py pmem2_integration -u 39 -g byte --force-enable pmemcheck -b debug
pmem2_integration/TEST39: SETUP (medium/debug/pmemcheck/byte)
Last 16 lines of /home/jmichal/work/pmdk/src/test/pmem2_integration/pmemcheck39.log below (whole file has 16 lines):
==1998050== pmemcheck-1.0, a simple persistent store checker
==1998050== Copyright (c) 2014-2020, Intel Corporation
==1998050== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==1998050== Command: /home/jmichal/work/pmdk/src/test/pmem2_integration/pmem2_integration test_map_from_existing /tmp/pmem2_integration_39/testfile
==1998050== Parent PID: 1998038
==1998050== 
==1998050== 
==1998050== Number of stores not made persistent: 1
==1998050== Stores not made persistent properly:
==1998050== [0]    at 0x401481A: _dl_fixup (dl-machine.h:239)
==1998050==    by 0x4001C3D: _dl_runtime_resolve_xsave (dl-trampoline.h:126)
==1998050==    by 0x402883: TEST_CASE_PROCESS (unittest.h:700)
==1998050==    by 0x405A7A: main (pmem2_integration.c:1030)
==1998050==     Address: 0x60f0c8       size: 8 state: DIRTY
==1998050== Total memory not made persistent: 8
==1998050== ERROR SUMMARY: 1 errors

src/libpmem2/map_posix.c line 593 at r2 (raw file):

		return ret;

	VALGRIND_REMOVE_PMEM_MAPPING(map_addr, map_len);

What if pmem2_map_from_existing() is called with src->type != PMEM2_SOURCE_FD so, in effect, the new mapping is not registered to Valgrind? So, I guess, it boils down to whether it is possible that the mapping is not registered (and I think it is possible)? And how Valgrind will react in this case?

src/test/unittest/ut_file.c line 375 at r2 (raw file):

	addr = ut_mmap(file, line, func, NULL, size, PROT_READ | PROT_WRITE,
		MAP_SHARED, fd, 0);
	return addr;

addr here is the most ignored variable I have seen for quite some time. xD
What is the purpose of existence? You may ask.

ldorau

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lplewa)

lplewa

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

src/libpmem2/map_posix.c line 593 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What if pmem2_map_from_existing() is called with src->type != PMEM2_SOURCE_FD so, in effect, the new mapping is not registered to Valgrind? So, I guess, it boils down to whether it is possible that the mapping is not registered (and I think it is possible)? And how Valgrind will react in this case?

Done.

src/test/unittest/ut_file.c line 375 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

addr here is the most ignored variable I have seen for quite some time. xD
What is the purpose of existence? You may ask.

Done

ldorau

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @lplewa)

lukaszstolarczuk · 2023-05-09T12:33:16Z

@lplewa, FYI, the CI checks failed

grom72

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @lplewa)

src/libpmem2/map_posix.c line 595 at r3 (raw file):

#ifndef _WIN32
	if (map->source.type == PMEM2_SOURCE_FD) {
		 VALGRIND_REMOVE_PMEM_MAPPING(map_addr, map_len);

Remove space before VALGRIND_REMOVE_PMEM_MAPPING

janekmi

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lplewa)

janekmi

Reviewed 1 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lplewa)

janekmi

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lplewa)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Maybe it is just me, but after applying these changes I still have one remaining problem:

$ ./RUNTESTS.py pmem2_integration -u 39 -g byte --force-enable pmemcheck -b debug
pmem2_integration/TEST39: SETUP (medium/debug/pmemcheck/byte)
Last 16 lines of /home/jmichal/work/pmdk/src/test/pmem2_integration/pmemcheck39.log below (whole file has 16 lines):
==1998050== pmemcheck-1.0, a simple persistent store checker
==1998050== Copyright (c) 2014-2020, Intel Corporation
==1998050== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==1998050== Command: /home/jmichal/work/pmdk/src/test/pmem2_integration/pmem2_integration test_map_from_existing /tmp/pmem2_integration_39/testfile
==1998050== Parent PID: 1998038
==1998050== 
==1998050== 
==1998050== Number of stores not made persistent: 1
==1998050== Stores not made persistent properly:
==1998050== [0]    at 0x401481A: _dl_fixup (dl-machine.h:239)
==1998050==    by 0x4001C3D: _dl_runtime_resolve_xsave (dl-trampoline.h:126)
==1998050==    by 0x402883: TEST_CASE_PROCESS (unittest.h:700)
==1998050==    by 0x405A7A: main (pmem2_integration.c:1030)
==1998050==     Address: 0x60f0c8       size: 8 state: DIRTY
==1998050== Total memory not made persistent: 8
==1998050== ERROR SUMMARY: 1 errors

A funny thing. @grom72 confirmed this fix works for him. So, the score is 2:1 for this commit being the fix.
I guess I am doing something atypical which causes this uncommon issue.

I think we can ignore it for the discussion around this commit.

janekmi

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lplewa)

ldorau

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lplewa)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

A funny thing. @grom72 confirmed this fix works for him. So, the score is 2:1 for this commit being the fix.
I guess I am doing something atypical which causes this uncommon issue.

I think we can ignore it for the discussion around this commit.

The score is 3:1 ;-) - both:

$ ./RUNTESTS.py pmem2_integration -u 39 --force-enable pmemcheck
$ ./RUNTESTS.py pmem2_integration -u 41 --force-enable pmemcheck

pass for me.

fixes: pmem#5597

wlemkows mentioned this pull request Apr 25, 2023

pmem2_integration/TEST[39,41]: failed pmemcheck #5597

Closed

ldorau requested changes Apr 25, 2023

View reviewed changes

janekmi added this to the 1.13 on GHA milestone Apr 27, 2023

grom72 requested changes Apr 27, 2023

View reviewed changes

grom72 reviewed Apr 27, 2023

View reviewed changes

lplewa force-pushed the stable-1.12 branch from c99fd78 to 8dcb79a Compare April 27, 2023 11:45

grom72 approved these changes Apr 27, 2023

View reviewed changes

ldorau approved these changes Apr 27, 2023

View reviewed changes

janekmi requested changes May 5, 2023

View reviewed changes

ldorau approved these changes May 8, 2023

View reviewed changes

lukaszstolarczuk mentioned this pull request May 8, 2023

Merge stable-1.13 into master #5619

Closed

1 task

lplewa force-pushed the stable-1.12 branch from 8dcb79a to e8d8ebf Compare May 9, 2023 10:41

lplewa changed the base branch from master to stable-1.13 May 9, 2023 10:42

lplewa commented May 9, 2023

View reviewed changes

ldorau requested a review from janekmi May 9, 2023 11:37

ldorau reviewed May 9, 2023

View reviewed changes

grom72 requested changes May 9, 2023

View reviewed changes

janekmi reviewed May 9, 2023

View reviewed changes

janekmi approved these changes May 9, 2023

View reviewed changes

ldorau approved these changes May 10, 2023

View reviewed changes

pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting

aea64b2

fixes: pmem#5597

ldorau force-pushed the stable-1.12 branch from e8d8ebf to aea64b2 Compare May 10, 2023 08:03

janekmi merged commit 8b16695 into pmem:stable-1.13 May 10, 2023
3 of 4 checks passed

janekmi mentioned this pull request May 12, 2023

Sporadic: pmem2_integration/TEST39 (medium/debug/pmemcheck/byte) #5634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting #5605

pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting #5605

lplewa commented Apr 25, 2023 •

edited by janekmi

Loading

wlemkows commented Apr 25, 2023

ldorau left a comment

ldorau left a comment

grom72 left a comment

grom72 left a comment

grom72 left a comment

grom72 left a comment

codecov bot commented Apr 27, 2023 •

edited

Loading

ldorau left a comment

janekmi left a comment

ldorau left a comment

lplewa left a comment

ldorau left a comment

lukaszstolarczuk commented May 9, 2023

grom72 left a comment

janekmi left a comment

janekmi left a comment

janekmi left a comment

janekmi left a comment

ldorau left a comment

pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting #5605

pmem2: fix valgrind instrumentation in pmem2_map_from_exisiting #5605

Conversation

lplewa commented Apr 25, 2023 • edited by janekmi Loading

wlemkows commented Apr 25, 2023

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

ldorau left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

lplewa left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

lukaszstolarczuk commented May 9, 2023

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

lplewa commented Apr 25, 2023 •

edited by janekmi

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading