LLNL / dataracebench

Data race benchmark suite for evaluating OpenMP correctness tools aimed to detect data races.
Other
67 stars 29 forks source link

Fix classification of some microkernels #151

Open jprotze opened 1 year ago

jprotze commented 1 year ago

The two renamed benchmarks do not contain a data race. DRB142 did not contain a data race. Removing the critical, adds back the intended data race.

LChenGit commented 1 year ago

Hi @jprotze,

Thank you for the PR. For micro-benchmarks/DRB129-mergeable-taskwait-orig-no.c, What if the code is the following? Does it contain data races? Thank you!

#include <omp.h>
#include <stdio.h>
void func(int *x)
{
#pragma omp task mergeable
    {
        (*x)++;
    }
#pragma omp taskwait
}

int main()
{
    int x = 2;
#pragma omp parallel
    {
        func(&x);
    }
    printf("%d\n", x);
    return 0;
}
LChenGit commented 1 year ago

Hi @jprotze Regarding micro-benchmarks/DRB142-acquirerelease-orig-yes.c, we think the original intention is to have thread 1 read variable x. Would changing y to x at line 41 correct the code and introduce the data race? Thank you!

jprotze commented 1 year ago

Hi @jprotze Regarding micro-benchmarks/DRB142-acquirerelease-orig-yes.c, we think the original intention is to have thread 1 read variable x. Would changing y to x at line 41 correct the code and introduce the data race? Thank you!

The comment describes a different intent. The implicit flush for the atomic signalling on y does not introduce flush-synchronization for reading x. The problem of the initial code is that the critical region guarding the read of x in the if-statement does actually add the flush-synchronization. Removing the critical avoids this flush-synchronization and implements the issue described in the header of the file.

jprotze commented 1 year ago

Your change suggested for DRB129 changes the test to a completely different test than documented in the comment. I think, the idea of the test is good. It is just not a data race.