manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

interrupt_status data races #255

Closed ghost closed 3 years ago

ghost commented 3 years ago

General information

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 3 years ago

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 3 years ago

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)

ghost commented 3 years ago

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