manodeep/Corrfunc

interrupt_status data races

Opened this issue · 3 comments

Deleted user commented

General information

  • Corrfunc version: Commit 6c63b73
  • platform: Ubuntu 20.04.2
  • installation method (pip/source/other?): Built from source

Issue description

Data races exist with the variables with the prefixes interrupt_status in the following files:

/Corrfunc/theory/vpf/countspheres_impl_float.c
/Corrfunc/theory/DD/countpairs_impl_double.c
/Corrfunc/theory/DDsmu/countpairs_s_mu_impl_double.c
/Corrfunc/theory/DDrppi/countpairs_rp_pi_impl_double.c
/Corrfunc/theory/wp/countpairs_wp_impl_double.c
/Corrfunc/theory/xi/countpairs_xi_impl_float.c
/Corrfunc/theory/DDrppi/countpairs_rp_pi_impl_float.c
/Corrfunc/theory/DDsmu/countpairs_s_mu_impl_float.c
/Corrfunc/theory/xi/countpairs_xi_impl_double.c
/Corrfunc/theory/vpf/countspheres_impl_double.c
/Corrfunc/theory/DD/countpairs_impl_float.c
/Corrfunc/theory/wp/countpairs_wp_impl_float.c

An example from countspheres_impl_float.c:

 37|{
 38|    fprintf(stderr,"Received signal = `%s' (signo = %d). Aborting \n",strsignal(signo), signo);
>39|    interrupt_status_vpf_float = EXIT_FAILURE;
 40|}
 41|

 249| /* loop through centers, placing each randomly */
 250| int ic=0;
>251| while(ic < nc && interrupt_status_vpf_float == EXIT_SUCCESS) {
 252|     if(options->verbose) {
 253|         my_progressbar(ic,&interrupted);

Here, we have a situation where one thread may attempt to read interrupt_status_vpf_float before another thread changes its value to EXIT_FAILURE.

This data race was discovered using the Coderrect Scanner https://coderrect.com/

lgarrison commented

Thanks; I think this is known/expected behavior. In our code, these variables are just "abort" flags, and we just care that we eventually quit if they are set; we don't need thread-level synchronization. @manodeep, do you agree?

manodeep commented

Thank you @markbcahill for your report. While the specific vpf example is single-threaded only, that pattern does exist in other multi-threaded routines that you have highlighted. However, as @lgarrison said - we just need the value to be updated by the thread for the desired functionality. (I thought I had put an inline comment to indicate this intentionality but I don't see it - ohh well)

Deleted user commented

Thank you for the quick reply! It can be hard to tell what is intended or not, since I'm not in the head of whoever originally programmed it