196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #206

johannct · 2024-09-12T20:32:24Z

Change Description

My PR includes a link to the issue that I am addressing

Solution Description

Code Quality

I have read the Contribution Guide
My code follows the code style of this project
My code builds (or compiles) cleanly without any errors or warnings
My code contains relevant comments and necessary documentation

Project-Specific Pull Request Checklists

Bug Fix Checklist

My fix includes a new test that breaks as a result of the bug (if possible)
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

New Feature Checklist

I have added or updated the docstrings associated with my feature using the NumPy docstring format
I have updated the tutorial to highlight my new feature (if appropriate)
I have added unit/End-to-End (E2E) test cases to cover my new feature
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

Documentation Change Checklist

Any updated docstrings use the NumPy docstring format

Build/CI Change Checklist

If required or optional dependencies have changed (including version numbers), I have updated the README to reflect this
If this is a new CI setup, I have added the associated badge to the README

Other Change Checklist

Any new or updated docstrings use the NumPy docstring format.
I have updated the tutorial to highlight my new feature (if appropriate)
I have added unit/End-to-End (E2E) test cases to cover any changes
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

codecov · 2024-09-12T20:35:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.65%. Comparing base (384f8d3) to head (5a396b6).
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
+ Coverage   66.29%   66.65%   +0.35%     
==========================================
  Files          50       50              
  Lines        6169     6229      +60     
  Branches      928      937       +9     
==========================================
+ Hits         4090     4152      +62     
+ Misses       2079     2077       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hdante

Overall: this pull request was linked to issue #197 but it fixes #196 instead.

hdante · 2024-09-12T20:54:03Z

CMakeLists.txt

+  message(STATUS "Building with thread sanitizing enabled.")
+  SET(GCC_COVERAGE_COMPILE_FLAGS "-g -O2 -fsanitize=thread")
+  SET(GCC_COVERAGE_LINK_FLAGS    "-fsanitize=thread")
+  SET( CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} ${GCC_COVERAGE_COMPILE_FLAGS}" )


There's a coding style inconsistency here: space between the parentheses and variable name vs. no space.

agreed, fixed in #7aee2ff

hdante · 2024-09-12T20:54:12Z

CMakeLists.txt

+  SET(GCC_COVERAGE_COMPILE_FLAGS "-g -O2 -fsanitize=thread")
+  SET(GCC_COVERAGE_LINK_FLAGS    "-fsanitize=thread")
+  SET( CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} ${GCC_COVERAGE_COMPILE_FLAGS}" )
+  SET( CMAKE_EXE_LINKER_FLAGS  "${CMAKE_EXE_LINKER_FLAGS} ${GCC_COVERAGE_LINK_FLAGS}" )


There's a coding style inconsistency here: space between the parentheses and variable name vs. no space.

agreed, fixed in #7aee2ff

hdante · 2024-09-12T20:55:27Z

CMakeLists.txt

 if(DEFINED ENV{INCLUDE_COVERAGE})
  message(STATUS "Building with code coverage enabled.")
-  SET(GCC_COVERAGE_COMPILE_FLAGS "-g -O0 -coverage -fprofile-arcs -ftest-coverage")
-  SET(GCC_COVERAGE_LINK_FLAGS    "-coverage -lgcov")
+  SET(GCC_COVERAGE_COMPILE_FLAGS "-g -O0 -coverage -fprofile-arcs -ftest-coverage -fPIC -pie")


Are explicit -fPIC and -pie really required here ? These flags are platform-dependent.

agreed. Fixed in #7aee2ff

hdante · 2024-09-12T21:00:03Z

src/lib/onesource.cpp

-  pdfcol1.vPDF.assign((PDFcol1loc), (PDFcol1loc+pdfcol1.size()));
-  pdfcol2.vPDF.assign((PDFcol2loc), (PDFcol2loc+pdfcol2.size()));
-  pdfmref.vPDF.assign((PDFmrefloc), (PDFmrefloc+pdfmref.size()));
+  pdfbayzg.vPDF.assign((PDFzloc), (PDFzloc + pdfbayzg.size()));


This is a coding style change unrelated to the pull request, should it have a different commit ?

It is done automatically by pre-commit, and comes from a previous merge from you that I forced, so I think we can get it through here now.

johannct · 2024-09-13T07:47:30Z

Overall: this pull request was linked to issue #197 but it fixes #196 instead.

damn..... I am not sure how to solve this.... I changed the name of the pull request, but did not change the branch name. When we fix we will be able to regenerate a branch for 197

hdante · 2024-09-13T20:40:49Z

Hello, Johann, the conclusion for these patches is that they work now, but when enabling the thread sanitizer and testing, I'm receiving what at first look seem to be false positive data races caused by my OpenMP library, which seems to be Intel's library implicitly installed by conda. Are you having any data race warnings ? I think I'll just open another issue that might be closed as invalid if the false positive is confirmed. The main question is then if it makes sense to have an option to enable the thread sanitizer if it doesn't play well with OpenMP.

The patch now looks good to me, if this repository allows forced pushes, I suggest squashing the commits with fixups into the original patch (96ab990) to avoid inserting commits with issues in the main branch.

https://stackoverflow.com/questions/33004809/can-i-use-thread-sanitizer-for-openmp-programs

johannct · 2024-09-14T06:55:36Z

Yes, I also saw a lot of warnings, starting with mag_gal, that I did not understand. A repeating one is the creation of oneSEDInt in each thread out of a given external single oneSED : the thread sanitizer seems to complain about copy constructor.... It may be worth trying suppression as my recollection is that the same warning was repeating itself over and over again. https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions
It does not cost much to leave the compilation option.
I did not understand what you have in mind with the last sentence about how to merge the fix.

hdante · 2024-09-16T19:09:39Z

Ok, so fully acknowledged. My last suggestion was to squash 3 of the proposed commits 96ab990, 8f06ccf and 7aee2ff into a single new commit containing the final proposed changes before merging them into the main branch.

hdante

Ack

…rallel-with-read-modify-write-operation-in-onesourcegeneratepdf

(squashing 3 commits)

This patch allows enabling the address sanitizer functionality present in some compilers to detect memory access errors when using the library. See: https://github.com/google/sanitizers/wiki/AddressSanitizer

…-read-modify-write-operation-in-onesourcegeneratepdf' of github.com:lephare-photoz/lephare into 197-reduction-operation-being-executed-in-parallel-with-read-modify-write-operation-in-onesourcegeneratepdf

johannct · 2024-09-17T08:54:09Z

@hdante dante looks like you need to redo a formal review :(

hdante · 2024-09-17T19:27:50Z

Johann, the resulting code is mostly ok, but the patch set as a whole ended up with some duplicated commits (both the squashed and the original commits are present) and 2 intermediate merges, so I decided to redo the complete pull request into another version:

https://github.com/hdante/lephare/tree/197-reduction-operation-being-executed-in-parallel-with-read-modify-write-operation-in-onesourcegeneratepdf

This branch only has 2 commits instead of 8 and is rebased on top of the main branch, so that no intermediate merges are needed. It results in the exact same code as this pull request. If you prefer I may open a pull request with that branch. After the commits were simplified I noticed that there might be some comments added in the makefile that are in the wrong place, so we'll need another review of that pull request.

johannct · 2024-09-17T19:30:39Z

darn.... so my squashing did not proceed as planned? Yes let's do as you suggest.

hdante · 2024-09-17T19:43:20Z

Ok, created pull request #209

…-read-modify-write-operation-in-onesourcegeneratepdf' into johannct-patch-1

Fix comments in CMakeLists.txt

johannct · 2024-09-17T20:31:15Z

This has been dealt with more properly in another PR

Johann Cohen-Tanugi added 2 commits September 12, 2024 19:37

add the missing arrays as private members in the reduction loop

7399581

add cmake directive for building with thread sanitizing

96ab990

johannct requested a review from hdante September 12, 2024 20:32

johannct self-assigned this Sep 12, 2024

johannct linked an issue Sep 12, 2024 that may be closed by this pull request

Reduction operation being executed in parallel with read-modify-write operation in onesource::generatePDF() #197

Open

3 tasks

add cmake directive for building with thread sanitizing

8f06ccf

hdante reviewed Sep 12, 2024

View reviewed changes

johannct linked an issue Sep 13, 2024 that may be closed by this pull request

Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #196

Closed

3 tasks

johannct changed the title ~~197 reduction operation being executed in parallel with read modify write operation in onesourcegeneratepdf~~ 196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() Sep 13, 2024

fix coding style and remove flags added by mistake

7aee2ff

hdante approved these changes Sep 16, 2024

View reviewed changes

johannct and others added 4 commits September 17, 2024 10:02

Merge branch 'main' into 197-reduction-operation-being-executed-in-pa…

8221cbd

…rallel-with-read-modify-write-operation-in-onesourcegeneratepdf

add cmake directive for building with thread sanitizing

3cefd8f

(squashing 3 commits)

Support address sanitizer builds

5f9c2d4

This patch allows enabling the address sanitizer functionality present in some compilers to detect memory access errors when using the library. See: https://github.com/google/sanitizers/wiki/AddressSanitizer

Merge branch '197-reduction-operation-being-executed-in-parallel-with…

68a11ac

…-read-modify-write-operation-in-onesourcegeneratepdf' of github.com:lephare-photoz/lephare into 197-reduction-operation-being-executed-in-parallel-with-read-modify-write-operation-in-onesourcegeneratepdf

johannct added 4 commits September 17, 2024 21:54

Fix comments in CMakeLists.txt

2c06835

Fix comments in CMakeLists.txt

43e923c

Merge branch '197-reduction-operation-being-executed-in-parallel-with…

d4b956a

…-read-modify-write-operation-in-onesourcegeneratepdf' into johannct-patch-1

Merge pull request #210 from lephare-photoz/johannct-patch-1

5a396b6

Fix comments in CMakeLists.txt

johannct closed this Sep 17, 2024

johannct removed a link to an issue Sep 17, 2024

Reduction operation being executed in parallel with read-modify-write operation in onesource::generatePDF() #197

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #206

196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #206

johannct commented Sep 12, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

hdante left a comment

hdante Sep 12, 2024

johannct Sep 13, 2024

hdante Sep 12, 2024

johannct Sep 13, 2024

hdante Sep 12, 2024

johannct Sep 13, 2024

hdante Sep 12, 2024

johannct Sep 13, 2024

johannct commented Sep 13, 2024 •

edited

Loading

hdante commented Sep 13, 2024

johannct commented Sep 14, 2024 •

edited

Loading

hdante commented Sep 16, 2024

hdante left a comment

johannct commented Sep 17, 2024

hdante commented Sep 17, 2024

johannct commented Sep 17, 2024

hdante commented Sep 17, 2024

johannct commented Sep 17, 2024

196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #206

196 Potential race condition in read-modify-write PDF vectors in parallel loop in onesource::generatePDF() #206

Conversation

johannct commented Sep 12, 2024

Change Description

Solution Description

Code Quality

Project-Specific Pull Request Checklists

Bug Fix Checklist

New Feature Checklist

Documentation Change Checklist

Build/CI Change Checklist

Other Change Checklist

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

hdante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannct commented Sep 13, 2024 • edited Loading

hdante commented Sep 13, 2024

johannct commented Sep 14, 2024 • edited Loading

hdante commented Sep 16, 2024

hdante left a comment

Choose a reason for hiding this comment

johannct commented Sep 17, 2024

hdante commented Sep 17, 2024

johannct commented Sep 17, 2024

hdante commented Sep 17, 2024

johannct commented Sep 17, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

johannct commented Sep 13, 2024 •

edited

Loading

johannct commented Sep 14, 2024 •

edited

Loading