fix openmp include and link issue on macos by fengyuentau · Pull Request #23095 · opencv/opencv

fengyuentau · 2023-01-04T07:24:10Z

Helpful notes:

OpenMP_CXX_INCLUDE_DIRS is defined since CMake >= 3.16.0.
OpenMP_CXX_LIBRARIES is defined since CMake >= 3.9.0.
For gnu openmp (libgomp, e.g. on Linux), OpenMP_CXX_INCLUDE_DIRS is null
regardless CMake version. Passing flag -fopenmp is enough.
For clang openmp (libomp, e.g. on macOS), OpenMP_CXX_INCLUDE_DIRS is not
null and need to include header and link libomp in addition to passing flag.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

force_builders=Custom
build_image:Custom=openmp:20.04
buildworker:Custom=linux-4

alalek · 2023-01-09T08:59:44Z

There is the same problems like: #21427 (comment)
Need to support both old and new versions.

fengyuentau · 2023-01-09T09:05:54Z

Need to support both old and new versions.

Sure, let me take a look into the old version. By the way, by "old version" do you mean versions <= 3.5?

alalek · 2023-01-09T09:14:39Z

OpenCV 4.0 has been released with CMake 3.5.1 support (version from Ubuntu 16.04).

BTW, we need to check for existence/definition of new variables (OpenMP_CXX_INCLUDE_DIRS), instead of CMake version.

fengyuentau · 2023-01-10T03:43:12Z

we need to check for existence/definition of new variables (OpenMP_CXX_INCLUDE_DIRS), instead of CMake version.

Latest commit should fix this issue. Tested with cmake==3.5.0: If OpenMP_CXX_INCLUDE_DIRS is empty, OPENMP_FOUND is set to false and pthreads is used in place of openmp.

alalek · 2023-01-10T10:05:14Z

If OpenMP_CXX_INCLUDE_DIRS is empty, OPENMP_FOUND is set to false and pthreads is used in place of openmp.

This is a regression "by definition". Because you broke OpenMP on configurations similar to Ubuntu 16.04.
Check "Custom" builder with OpenMP:

fengyuentau · 2023-01-10T10:52:39Z

I found that gnu gcc links libgomp by default on Linux, and the gnu openmp library is kind of integrated into the gnu gcc, so passing flag -fopenmp is enough and feasible for gnu gcc.

But this is not the case for clang on macOS. clang on macOS treats libomp (clang openmp library) as a third-party library and it needs the specified header and library. So does it make sense if I add condition if(APPLE) to make the change only valid for macOS?

alalek · 2023-01-10T10:58:36Z

if I add condition if(APPLE)

No.
Read above:

we need to check for existence/definition of new variables (OpenMP_CXX_INCLUDE_DIRS)

fengyuentau · 2023-01-10T11:04:35Z

I need more information about the return of find_package(OpenMP) of different CMake versions. Do you know by any chance when OpenMP_CXX_INCLUDE_DIRS is empty? Does it relate only to CMake versions? Or relate to platforms as well?

alalek · 2023-01-10T11:08:33Z

In CMake 3.5.1 OpenMP_CXX_INCLUDE_DIRS is not defined at all (not used). E.g., see "vars" log of CMake output of Custom builder.

Again, refer to comments from previous PR: #21427 (comment)

To track behavior changes of CMake scripts you need to check history of CMake.

fengyuentau · 2023-01-12T08:27:11Z

I found WITH_OPENMP is also broken with cmake==3.5.1 (maybe even cmake<=3.5.1) on Ubuntu 20.04. pthreads is used instead in this case without the changes in this pull request. Let me push a commit to see whether it works on Ubuntu 16.04 with cmake==3.5.1.

fengyuentau · 2023-01-12T14:54:43Z

It shows OpenMP is used on openmp:16.04 with latest commit: https://pullrequest.opencv.org/buildbot/builders/precommit_custom_linux/builds/100176/steps/cmake/logs/stdio.

Let me take a look at cmake==3.5.1 on Ubuntu 20.04.

fengyuentau · 2023-01-13T09:42:28Z

pthreads is used instead in this case without the changes in this pull request. Let me push a commit to see whether it works on Ubuntu 16.04 with cmake==3.5.1.

This is actually caused by a typo I made to the CMake option. It is good now. Feel free to review this pull request @alalek

alalek · 2023-01-13T12:59:49Z

modules/core/CMakeLists.txt

  ocv_target_link_libraries(${the_module} LINK_PRIVATE "${HPX_LIBRARIES}")
 endif()

+if(WITH_OPENMP AND DEFINED OpenMP_CXX_INCLUDE_DIRS AND OpenMP_CXX_INCLUDE_DIRS)


WITH_OPENMP

HAVE_OPENMP

OpenMP_CXX_INCLUDE_DIRS

Empty OpenMP_CXX_LIBRARIES would trigger CMake error below with missing argument, so check should use OpenMP_CXX_LIBRARIES instead.

Okay, let me bake another commit for this.

Empty OpenMP_CXX_LIBRARIES would trigger CMake error below with missing argument, so check should use OpenMP_CXX_LIBRARIES instead.

I found that OpenMP_CXX_LIBRARIES is defined since CMake>=3.9.0, while OpenMP_CXX_INCLUDE_DIRS is defined since CMake>=3.16.0. So OpenMP_CXX_LIBRARIES is never empty when OpenMP_CXX_INCLUDE_DIRS has value. It actually behaves exactly like this in my experiences below:

Linux (Ubuntu 20.04)

cmake-3.5.1 inc: no def, lib: no def

cmake-3.9.6 inc: no def, lib: def +value

cmake-3.15.7 inc: no def, lib: def + value

cmake-3.16.0 inc: def (no value), lib: def + value

Update:

I use HAVE_OPENMP and OpenMP_CXX_LIBRARIES as condition for linking in the latest commit. Basically this patch of code is triggered when CMake >= 3.16.0 and it is on clang.

alalek · 2023-01-13T13:05:16Z

cmake/OpenCVFindFrameworks.cmake

    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
+    # NOTES:
+    # 1. For CMake <= 3.5.1, OpenMP_CXX_INCLUDE_DIRS is not defined.


3.5.1

This is not correct.
This is a minimal CMake version on which we take a look look for OpenCV 4.x.

https://cmake.org/cmake/help/v3.6/module/FindOpenMP.html

still no _INCLUDE_DIRS too.

I checked CMake docs and found _INCLUDE_DIRS is defined since CMake>=3.16.0. See https://cmake.org/cmake/help/v3.16/module/FindOpenMP.html#backward-compatibility.

alalek · 2023-01-15T19:12:05Z

cmake/OpenCVFindFrameworks.cmake

+    # NOTES:
+    # 1. OpenMP_CXX_INCLUDE_DIRS is defined since CMake >= 3.16.0.
+    # 2. OpenMP_CXX_LIBRARIES is defined since CMake >= 3.9.0.
+    # 2. For gnu openmp (libgomp, e.g. on Linux), OpenMP_CXX_INCLUDE_DIRS is null


double 2.

Not sure if we really need such detailed comment here (especially not sure about its accuracy).

OpenMP_CXX_LIBRARIES and OpenMP_CXX_INCLUDE_DIRS are available with modern CMake scripts only. We use checks to detect that case.

Okay, I will copy-paste all the comments in the first comment of this PR and delete them from the code.

alalek · 2023-01-15T19:16:11Z

modules/core/CMakeLists.txt

  ocv_target_link_libraries(${the_module} LINK_PRIVATE "${HPX_LIBRARIES}")
 endif()

+if(HAVE_OPENMP AND DEFINED OpenMP_CXX_INCLUDE_DIRS AND OpenMP_CXX_LIBRARIES)


OpenMP_CXX_INCLUDE_DIRS AND OpenMP_CXX_LIBRARIES

mess of different variables

According to my test above, there is a case when _LIBRARIES is defined and has value but _INCLUDE_DIRS is not defined at all (CMake >= 3.9, <= 3.15). This condition is basically ensuring the old behavior is not changed, which is OpenMP_CXX_LIBRARIES will not be linked until OpenMP_CXX_INCLUDE_DIRS is defined.

Do you suggest using OpenMP_CXX_LIBRARIES in replace of OpenMP_CXX_INCLUDE_DIRS for definition checking?

Update:

It is now using if(HAVE_OPENMP AND DEFINED OpenMP_CXX_INCLUDE_DIRS AND OpenMP_CXX_INCLUDE_DIRS AND OpenMP_CXX_LIBRARIES) as conditions for linking libomp.

No need to use other variables here.
Logic should protect usage from empty/non-defined values. No more, no less.

if(HAVE_OPENMP AND DEFINED OpenMP_CXX_LIBRARIES AND OpenMP_CXX_LIBRARIES) ocv_target_link_libraries(${the_module} LINK_PRIVATE "${OpenMP_CXX_LIBRARIES}") endif()

Okay, done with latest commit.

alalek

LGTM 👍

* fix openmp include and link issue on macos * turn off have_openmp if OpenMP_CXX_INCLUDE_DIRS is empty * test commit * use condition HAVE_OPENMP and OpenMP_CXX_LIBRARIES for linking * remove trailing whitespace * remove notes * update conditions * use OpenMP_CXX_LIBRARIES for linking

themightyoarfish · 2025-08-07T15:24:32Z

On macos, i am getting

Undefined symbols for architecture arm64:
  "_omp_get_max_threads", referenced from:
      Eigen::internal::manage_multi_threading(Eigen::Action, int*) in dls.cpp.o
  "_omp_get_num_threads", referenced from:
      Eigen::internal::general_matrix_matrix_product<long, double, 1, false, double, 1, false, 0, 1>::run(long, long, long, double const*, long, double const*, long, double*, long, long, double, Eigen::internal::level3_blocking<double, double>&, Eigen::internal::GemmParallelInfo<long>*) in essential_solver.cpp.o
  "_omp_get_thread_num", referenced from:
      Eigen::internal::general_matrix_matrix_product<long, double, 1, false, double, 1, false, 0, 1>::run(long, long, long, double const*, long, double const*, long, double*, long, long, double, Eigen::internal::level3_blocking<double, double>&, Eigen::internal::GemmParallelInfo<long>*) in essential_solver.cpp.o
ld: symbol(s) not found for architecture arm64

when building the 4.12.0 tag

themightyoarfish · 2025-08-07T15:44:36Z

specifically it fails linking calib3d, because the Openmp library is not getting linked, because the core module links it privately, but it has to be PUBLIC I think.

fix openmp include and link issue on macos

88532ba

asmorkalov added category: build/install platform: ios/osx labels Jan 9, 2023

asmorkalov added this to the 4.8.0 milestone Jan 9, 2023

asmorkalov requested a review from alalek January 9, 2023 07:50

turn off have_openmp if OpenMP_CXX_INCLUDE_DIRS is empty

61eb3d7

test commit

d826869

alalek reviewed Jan 13, 2023

View reviewed changes

fengyuentau added 2 commits January 15, 2023 20:37

use condition HAVE_OPENMP and OpenMP_CXX_LIBRARIES for linking

eb25e4a

remove trailing whitespace

c184726

alalek reviewed Jan 15, 2023

View reviewed changes

fengyuentau added 3 commits January 16, 2023 11:45

remove notes

eefb25f

update conditions

8d553b4

use OpenMP_CXX_LIBRARIES for linking

c0972d4

alalek approved these changes Jan 16, 2023

View reviewed changes

alalek mentioned this pull request Jan 16, 2023

Make sure openmp include dir is added to compiler include dirs #21427

Closed

6 tasks

alalek merged commit c63d79c into opencv:4.x Jan 16, 2023

fengyuentau deleted the fix_omp_macos branch January 16, 2023 09:46

alalek mentioned this pull request Jan 18, 2023

(5.x) Merge 4.x #23149

Merged

	Linux (Ubuntu 20.04)
cmake-3.5.1	inc: no def, lib: no def
cmake-3.9.6	inc: no def, lib: def +value
cmake-3.15.7	inc: no def, lib: def + value
cmake-3.16.0	inc: def (no value), lib: def + value

Uh oh!

Conversation

fengyuentau commented Jan 4, 2023 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek commented Jan 9, 2023

Uh oh!

fengyuentau commented Jan 9, 2023

Uh oh!

alalek commented Jan 9, 2023

Uh oh!

fengyuentau commented Jan 10, 2023

Uh oh!

alalek commented Jan 10, 2023

Uh oh!

fengyuentau commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Jan 10, 2023

Uh oh!

fengyuentau commented Jan 10, 2023

Uh oh!

alalek commented Jan 10, 2023

Uh oh!

fengyuentau commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fengyuentau commented Jan 12, 2023

Uh oh!

fengyuentau commented Jan 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengyuentau Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengyuentau Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

themightyoarfish commented Aug 7, 2025

Uh oh!

themightyoarfish commented Aug 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fengyuentau commented Jan 4, 2023 •

edited by alalek

Loading

fengyuentau commented Jan 10, 2023 •

edited

Loading

fengyuentau commented Jan 12, 2023 •

edited

Loading

fengyuentau Jan 15, 2023 •

edited

Loading

fengyuentau Jan 16, 2023 •

edited

Loading