In D87084#2254700, @jdoerfert wrote:

Other places need to be updated too: data_sharing_init_stack_common for one.

Do we actually use this array? What happens if you make it a single element and update the function above?

In D83268#2135930, @JonChesterfield wrote:

Aside from the API stability concern this looks uncontentious. Removes dead arguments, generally makes things simpler. Thus LGTM.

@Hahnfeld @ABataev - are you sufficiently persuaded that preserving the current interface is not worth the development cost?

This is definitely not NFC and breaks API compatibility (but apparently nobody cares anymore?).

LGTM

The summary also talks about tools/multiplex/tests/*, is that information obsolete?

LGTM

LGTM

In D82963#2128507, @saiislam wrote:

Thanks @Hahnfeld. I realized that LLVM_MAJOR_VERSION was neither getting set in OpenMPTesting.cmake nor was it
inheriting it from anywhere else. So, OPENMP_TEST_COMPILER_VERSION_MAJOR was also getting set as empty, which
was getting propagated to lit by config.test_compiler_features. That is why "clang-11" was not getting recognized
as a valid target by lit-unsupported (though clang-11.0.0 would have worked). This change should fix this issue.

In D82718#2120414, @ye-luo wrote:

In D82718#2120235, @Hahnfeld wrote:

(In general, all patches must be sent to the respective -commits list. This also makes feedback more likely.)

Sorry, I was not aware of -commits list. What is it for exclusively? If there is policy or instruction , please point me.
I added OpenMP project tag. I you watch OpenMP project on differential. I expect you get notifications. Is it not the case?

(In general, all patches must be sent to the respective -commits list. This also makes feedback more likely.)

In D76012#2100421, @sylvestre.ledru wrote:

@protze.joachim @jdoerfert ompt-multiplex.h is installed by default in /usr/include/
others opemp headers are installed in /usr/include/openmp/

LIBOMP_HEADERS_INSTALL_PATH is the internal compiler where for example omp.h. I think the header tool is of relevance outside of Clang, so IMO it should go the general include/ directory.

I don't feel comfortable reviewing this change, maybe @phosek can take a look.

Does -fopenmp-version work with the Intel Compiler? How will this work if the GCC eventually gets support for OpenMP 5.0?

LGTM as well

In D78566#1995119, @jdenny wrote:

I just noticed another issue: ompt tests are skipped if FileCheck is not available. This is configured in openmp/runtime/test/lit.cfg. However, other openmp test suites that use FileCheck don't bother. Why is ompt different?

This patch makes not visible in all of the openmp project's test
suites. In all but libomptarget/test, it should be possible for a
test author to insert not before a use of the lit substitution for
running a test program.

@thakis thanks for taking care so quickly!

In D76780#1942190, @bryanpkc wrote:

In D76780#1942144, @bryanpkc wrote:

Why doesn't the KMP_MB after the store to th_next_waiting guarantee that the unblocked thread sees that store?

Answering my own question, the ARM architecture reference manual states that the DMB instruction ensures that all affected memory accesses by the PE executing the DMB that appear in program order before the DMB and those which originate from a different PE, ...which have been Observed-by the PE before the DMB is executed, are Observed-by each PE, ...before any affected memory accesses that appear in program order after the DMB are Observed-by that PE.

I think this means, in this case, that the store of FALSE to th_spin_here is observed in the correct order only by the releasing thread that issued the DMB. Other threads (e.g. the spinning thread) could still see the updates to th_spin_here and th_next_waiting out of order, unless they also issue DMB, which is the fix in this patch.

Yes, this looks related to memory consistency. As far as I understand the threads synchronize on th_spin_here, so this is guaranteed to be updated. Any other write before this in __kmp_release_queuing_lock is not guaranteed to be synchronized by a weak memory model. This includes th_next_waiting (which triggers the assertion), but also writes by the user application. That's particularly bad because this should be taken care of by the runtime!

In D75001#1922789, @jdoerfert wrote:

In D75001#1891284, @Hahnfeld wrote:

In D75001#1887876, @jdoerfert wrote:

I like this way better. I was hoping we could do it in our cmake only :)

Give others a day or so to comment before your commit but I'm fine with this.

Well, that doesn't really work if openmp-commits is only subscribed on commit. That said, the solution is a bit ugly but I don't have an alternative right now.

What is the problem with openmp-commits here? I got the emails, didn't you?

In D75001#1887876, @jdoerfert wrote:

I like this way better. I was hoping we could do it in our cmake only :)

Give others a day or so to comment before your commit but I'm fine with this.

In D74258#1865532, @JonChesterfield wrote:

Unless there are good reasons to switch, I'd object because it requires newer versions of compilers than many HPC systems have.

I disagree. The use case we would be supporting is:

• building llvm libomptarget from source
• using it with a binary llvm dist, or some other toolchain
• won't download or build a newer toolchain

This should have a RFC on openmp-dev. As far as I understand the linked patch, the aim is to use std::make_unique which is just syntactic sugar IMO. Unless there are good reasons to switch, I'd object because it requires newer versions of compilers than many HPC systems have.

LGTM from the code, I didn't test that the issue is indeed fixed on i386 though

@protze.joachim for 64 bit, we still need at least 5 threads IIRC

As a general comment, I think this patch should be split into at least two separate ones:

1. Load plugins from the same directory as libomptarget.so is found.
2. Quick fail mechanism, although I'm not sure how much this saves? In most cases, we won't have the plugins for a foreign architecture (PPC on x86) anyway, that might only be a concern for accelerators.

In D73249#1835551, @mgorny wrote:

In D73249#1835526, @Hahnfeld wrote:

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/.

If by 'guaranteed' you mean in the current code, then I've tested this patch with it being both true and false.

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

LG

In D70412#1821136, @protze.joachim wrote:

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

This revision was not accepted before being committed!

Can you please also submit a patch to libc++ where we copied this code from?

In D71988#1799064, @jdoerfert wrote:

I would expect that some versioning is needed..

Versioning is fine with me. Versioning the runtime would be even preferable.

Yes, this must not be changed for compatibility reasons. If this really brings a benefit, we need new entry functions and update the compiler(s).

In D71340#1779446, @simoll wrote:

To clarify: this patch is not intended to be committed as-is but gives an overview of the required changes. It's the a patch of a series for the VE backend and includes all the earlier patches.

In D70804#1763443, @bryanpkc wrote:

In D70804#1763289, @ABataev wrote:

Tests are required

The tests for this were added in D30644, but they are currently failing on AArch64 due to D32035. This patch allows those tests to pass again. What further testing should I add?

Random drive-by comments to parts of the patch, I won't review in full.

Testing of openmp is currently very lacking, but I don't think this change will apply cleanly anymore...

Going through my old revisions, is this still something that we want?

I won't continue work on this.

Heads up that this change broke compatibility with old binaries. Not sure if that is something worth to fix, but it should probably at least be spelt out somewhere...

In D66107#1666702, @Dor1s wrote:

In D66107#1666574, @Hahnfeld wrote:

I think the bots are also green, so it might be just related to how I build Clang (with libc++, for example). I'm half way through building ToT with GCC, that should give insight whether it's related to my system or my configuration.

Actually, some ARM bots got broken, so I've uploaded a fix: https://reviews.llvm.org/D67458

Just curious: So the number of new features and coverage edges does not need to be the same? I also have "15 new freatures" for CHECK2.

Yes, features include coverage edges + additional signal (e.g. value profiling), that's why the number is different and why number of features can differ between platforms.

In D66107#1666569, @Dor1s wrote:

Hm, doesn't fail for me, but I guess the feature detection might be platform-dependent to some extent, so I'm fine with replacing the number of the features with a regex. Do you want to upload a change, or should I?

The new test is failing for me because CHECK1 is not satisfied. Instead the line says MERGE-OUTER: 3 new files with 12 new features added; 11 new coverage edges (instead of 11 new features). I'm currently investigating what's wrong here, let me know if you have an idea.

LGTM

LG iff the following is correct: The leak is when dyn_cast can't cast and returns null, right? If so, please add into the summary (and do so for future revisions, it will speed up review).

The code itself looks good to me (it should probably be clang-formated). If others have comments, please raise them now.

In D67298#1662222, @serge-sans-paille wrote:

Successfully tested on my box! I'd just use

diff
- UNSUPPORTED: asan-dynamic-runtime
+REQUIRES: x86_64-target-arch && !android && !asan-dynamic-runtime

if that works on your side too.

For the record, I don't think this would have worked: I do have libstdc++ available on my system (I just choose SANITIZER_CXX_ABI=libcxxabi for other reasons) and passing -stdlib=libstdc++ when building the test doesn't help as I've already said in D67298. Moreover, there's nothing in the test that requires libstdc++, it just happens to pass due to many chained implications, and restricting a test without understanding the root cause is probably no good idea.

In D67298#1662107, @serge-sans-paille wrote:

And that's what I don't understand

My fault for not being clear enough. Let me elaborate the original faulty use case:

You've got an application written in C, compiled with asan, that dlopens a library written in C++, linked to libstdc++
When the application starts, asan installs a few handler based on the loaded symbols. It used to install a handler that points to itself for __cxa_throw because the symbol is not present in the binary.

I think I just figured out that the original problem is about static runtime, so it doesn't make sense to run the test with dynamic asan.

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error.

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error. By defining __cxa_throw in the c file, you remove the origin of the bug, so sure it works, but it no longer tests the faulty situation. Maybe there's a symbol caught by asan and defined in both libcxx and libstdc++?

I've posted a change that makes the test work for me in D67298, please take a look.

Also, there should be a summary of the changes in here.

In D63877#1656860, @serge-sans-paille wrote:

Is that summary correct?

It is :-)

In D63877#1656135, @serge-sans-paille wrote:

@Hahnfeld : I understand your concern. However this test case is meant to test handling of stdlib exceptions, so it doesn't look strange to disable it if stdlib is not used, right? [...]

This changes error messages, so I'd say it's not NFC.

In D63877#1641054, @serge-sans-paille wrote:

@Hahnfeld : I failed to reproduce the issue locally, but I assume the following patch may solve your issue:

--- a/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
+++ b/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
@@ -5,7 +5,7 @@
 //
 // CHECK: {{.*}}AddressSanitizer: failed to intercept '__cxa_{{.*}}throw{{.*}}'
 //
-// REQUIRES: x86_64-target-arch && !android
+// REQUIRES: x86_64-target-arch && !android && !cxxabi

can you give it a try?

LG, thanks for the changes.

LG

Please submit the test changes unrelated to the code changes in a separate patch!