With this patch, I would expect runtime/test/ompt/tasks/dependences_mutexinoutset.c to succeed. Running the test with gcc-9/10, this test reports the dependency as ompt_dependence_type_inout and not ompt_dependence_type_mutexinoutset. Since the OMPT interface just translates the internal values, I assume something is wrong with the flags of the dependency.

The GOMP wrapper makes sense to me.
Several inline comments for the test.

Thanks for submitting this patch!

Since I spent hours to hunt down several race conditions in libomp in the last months, please fix races immediately, when they are pointed out. There is no such thing as a benign race!

In D84470#2191752, @ABataev wrote:

Add a test

Not sure whether greg still plans to work on this. Anyway a inline comment to the PATH_MAX

ping

ping

Updated to be a bit more specific, where the external version is used.

In D84557#2178384, @jdoerfert wrote:

Thanks. Could you ask @hans to include this in the release (via your bug) if you haven't already?

Thanks for reviewing.

You are basically right. In implementation, a function is generated for every mapper to do all internal mapping. More details can be found at https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx

stderr is unbuffered by default. Are we sure this behavior is not changed when the output is redirected into a file, as for example during execution in a batch job? What were the worries about having the fflush there?

From my perspective, the declare_mapper_target.cpp code is semantically equivalent to:

I included this change in D84557, where I enable nvptx64 testing for most libomptarget tests.

afa1afd4108d changed the minimum required version in all subdirectories, including openmp

The cmake version was bumped to 3.14

I carefully made sure, that the freshly built clang was used to execute the test. I opened https://bugs.llvm.org/show_bug.cgi?id=46836 to track the issue and made it release blocker.

This patch breaks compilation of previously working code.

I'm looking through the tests in this directory and try to run them on a GPU.

I'm not enough invested in the compiler implementation to judge this patch.
I can confirm that this patch fixes the segmentation fault I have seen.

After harbormaster highlighted the issue, this is the patch to solve the issue.

I'm executing:

$ /usr/bin/python bin/llvm-lit -vv -a projects/openmp/libomptarget/test/offloading/target_depend_nowait.cpp

which executes:

"./bin/clang++" "-fopenmp" "-pthread" "-fno-experimental-isel" "-I" "../openmp/libomptarget/test" "-I" "./projects/openmp/libomptarget/../runtime/src" "-L" "./lib" "-fopenmp-targets=x86_64-pc-linux-gnu" "../openmp/libomptarget/test/offloading/target_depend_nowait.cpp" "-o" "./projects/openmp/libomptarget/test/offloading/Output/target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu"

Ok, the bisecting did not really reveal anything new. The test fails with and without the env var for 140ab574 , starting with 537b16e9 the test succeeds with env LIBOMPTARGET_DEBUG=1

For the commit of this patch, the test fails with and without env LIBOMPTARGET_DEBUG=1. I'm using a release build, but have -DLIBOMPTARGET_ENABLE_DEBUG=on. This allows to activate debug output by setting the env variable.

The other tests failed after the commit. They started to succeed with various commits.
At above mentioned commit, only this single test fails.

Starting with the commit of this patch, the libomptarget test llvm-project/openmp/libomptarget/test/offloading/target_depend_nowait.cpp fails. Here is a stacktrace of the segfault:

$ gdb target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu
(gdb) run
...
Program received signal SIGSEGV, Segmentation fault.
(gdb) bt
#0  0x0000000000400fcf in .omp_outlined._debug__ (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:22
#1  0x0000000000401e8d in .omp_outlined..23 (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:18
#2  0x00002aaaab574213 in __kmp_invoke_microtask () from libomp.so
#3  0x00002aaaab51338e in __kmp_invoke_task_func () from libomp.so
#4  0x00002aaaab5126bf in __kmp_launch_thread () from libomp.so
#5  0x00002aaaab55d3d0 in __kmp_launch_worker(void*) () from libomp.so
#6  0x00002aaaabbd6e65 in start_thread () from libpthread.so.0
#7  0x00002aaaabee988d in clone () from libc.so.6

declare_mapper_target.cpp still fails for me consistently in RUN line 2, so for the nvptx version. I execute on x86_64 with Tesla V100 and cuda 10.0.
When I execute the test with export LIBOMPTARGET_DEBUG=1, the test succeeds.
In case of failure, the test prints Sum = 1024, in case of success, the test prints Sum = 2048 as expected.

Thanks for the update, lgtm

Sorry, I was looking at the date and thought the commit was 2 days old instead of 1 year :)
You are right, the test started to fail after the D67833 commit.

@DavidTruby please change the default to false and I'll accept the patch.

I think this export directory structure is a remainder from the original configure based Intel/OpenMP runtime.
@jlpeyton @AndreyChurbanov is this step still needed?

What would be needed to remove this strange copying step? Do the tests really rely on these directories?
I could run the tests successfully with cmake -DLIBOMP_COPY_EXPORTS=FALSE.

The test fails for me with segfault in line 22.

On a different system, I also get warnings for CMP0074.
The two policies were introduced with cmake 3.12. As long as we don't bump the required cmake version to at least 3.12, we need to set the policies.

I think, the better solution is to use the _pc version of the access functions to use the "real" access location.

I reverted the commit, because all buildbots failed for these tests:

TEST 'ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset' FAILED
TEST 'ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset2' FAILED

In D83625#2152829, @dvyukov wrote:

In D83625#2150120, @protze.joachim wrote:

I agree, that the patch changes the transitive suppression of reports. Unfortunately, this would bring back the overhead of reconstructing the remote stack trace.

As a follow-up patch I was thinking about adding a suppress_equal_location flag.
I would only compare the local stack trace with previously reported stack traces in racy_stacks.
Initially I was thinking about suppress_equal_pc, but especially for wrapper functions, the top pc is function pointer of the wrapper.

Right, it can do more harm.

After a pull from master, all tests were successful with this patch.

I added a missing a return statement

I agree, that the patch changes the transitive suppression of reports. Unfortunately, this would bring back the overhead of reconstructing the remote stack trace.

I moved the loops into functions.
I think for the previous controlflow, AddRacyStacks was necessary because of the early return false in line 475. I removed it, as the functions now add the entry directly to the vector.

Addressed @Hahnfeld 's comments

I have no idea, what Harbormaster is trying to tell me.

In other tests we start gcc versions with gcc-4.
I added those flags for the task reduction case without explicitly testing gcc-4/gcc-5

Sorry, in my previous update I missed to include the other changes (diff to the wrong commit)

Does clang-11 now default to 50 behavior?
In this case, we can remove -fopenmp-version=50 from the tests, right?

My comment was just an observation based on the code modification. I have not much understanding of the implications of this flag other than the few lines I read in the documentation and the comparison @tianshilei1992 posted.

The intention was to check the return address for the first two tasks. Unfortunately, the CHECK did not capture the return address of the second task creation, but instead compared to the return address of the first task.

I found another issue with this test (addressing Saiyedul's error). I'll post an updated patch in a minute

I think D82267 should fix the issue for dependence.c

This patch drops the CU_CTX_SCHED_BLOCKING_SYNC property currently selected for the context. Is this intended? Should we add another function call to request this behavior for the primary context?

In D81891#2112179, @hbae wrote:

The change looks good to me.
Can you check why the test fails? It passes on my test system.

Rebase on current master