This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget][NFC] Link plugins with threads support library due to std::call_once usage.
ClosedPublic

Authored by vzakhari on Jan 27 2021, 3:48 PM.

Details

Summary

The title is self-descriptive. There are some details here: https://reviews.llvm.org/D93727#2524458

I was going to assert that the threads support library is found in openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake, but I guess there may be plugins that do not use Debug.h, so I left it undefined what happens, if the library is not found.

Diff Detail

Event Timeline

vzakhari created this revision.Jan 27 2021, 3:48 PM
vzakhari requested review of this revision.Jan 27 2021, 3:48 PM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert accepted this revision.Jan 27 2021, 5:11 PM

This is not NFC but other than that reasonable. LG, @grokos ?

This revision is now accepted and ready to land.Jan 27 2021, 5:11 PM
grokos accepted this revision.Jan 27 2021, 6:26 PM

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

We need to backport this to 12, right?

We need to backport this to 12, right?

I guess so. Otherwise users may get failures that are really hard to explain...

vzakhari added a comment.EditedJan 27 2021, 7:14 PM

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

Do you mean I can ignore the pre-merge checks fail? I do not see how my changes could have affected libomp build.

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

Do you mean I can ignore the pre-merge checks fail? I do not see how my changes could have affected libomp build.

looks to me like that is the C++ dependence problem we discussed this morning. I don't think this patch caused it so I would go ahead.

This revision was landed with ongoing or failed builds.Jan 27 2021, 7:26 PM
This revision was automatically updated to reflect the committed changes.

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

Do you mean I can ignore the pre-merge checks fail? I do not see how my changes could have affected libomp build.

looks to me like that is the C++ dependence problem we discussed this morning. I don't think this patch caused it so I would go ahead.

Thank you! Merged.