Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Incomplete pkg-config module when using OpenMP. #1224

Closed
FtZPetruska opened this issue Mar 18, 2023 · 3 comments · Fixed by #1228
Closed

Incomplete pkg-config module when using OpenMP. #1224

FtZPetruska opened this issue Mar 18, 2023 · 3 comments · Fixed by #1228

Comments

@FtZPetruska
Copy link
Contributor

FluidSynth version

Execute fluidsynth --version and provide the output.

FluidSynth runtime version 2.3.2
Copyright (C) 2000-2023 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of Creative Technology Ltd.

FluidSynth executable version 2.3.2
Sample type=double

Describe the bug

Provide a clear and concise description of the current situation, e.g. how the bug manifests.

When building a programs against fluidsynth using the flags provided by pkg-config, the link step fails with undefined references to OpenMP.

Expected behavior

Provide a clear and concise description of what you expected to happen.

The program builds successfully.

Steps to reproduce

Please explain the steps required to duplicate the issue, esp. if you are able to provide a sample application. E.g. how to start fluidsynth, what shell commands to enter, what midi events to send, etc.

  • Build fluidsynth statically with OpenMP enabled, OpenMP needs to be an external library (such as LLVM's libomp)
  • Try to link a program using pkg-config --libs --static fluidsynth
  • Linking fails due to undefined references to OpenMP functions

Additional context

If you are able to illustrate the bug with an example, please provide simple
source code below or as attached file. List any other information that is relevant to your issue, e.g. stack traces, related issues, build logs, suggestions on how to fix, links to related discussions at fluid-dev, etc.

I noticed the following snippet in cmake_admin/PkgConfigHelpers.cmake:

get_target_property (_libs ${target} INTERFACE_LINK_LIBRARIES)
set(_cleanlibs)
foreach(_lib IN LISTS _libs)
    if (TARGET ${_lib})
        # All the imported PkgConfig target are explicitly added to PC_REQUIRES_PRIV.
        # Do not duplicate them into the Libs.private section, as they will be already part of Requires.private
    else()
        list(APPEND _cleanlibs ${_lib})
    endif()
endforeach()

This excludes all targets, which includes OpenMP_C.

Similarly, this snippet will add PulseAudio's libraries since they aren't linked through a target, despite PulseAudio providing a pkg-config module.

A possible solution could be to build a list similar to PC_REQUIRES_PRIV that contains libraries which do not have pkg-config module files, instead of relying on the INTERFACE_LINK_LIBRARIES of the libfluidsynth target. For example:

if ( TARGET OpenMP::OpenMP_C AND (( NOT OpenMP_C_SPEC_DATE LESS "201307" ) OR NOT ( OpenMP_C_VERSION VERSION_LESS "4.0" )) )
    set ( HAVE_OPENMP 1 )
    list ( APPEND PC_LIBS_PRIV ${OpenMP_C_LIBRARIES} )
else()

Then in the helper macro, we only need to remove duplicate and apply the same process as currently:

list ( REMOVE_DUPLICATES PC_LIBS_PRIV )
set (LIBS_PRIVATE ${PC_LIBS_PRIV})
# make a copy
set ( LIBS_PRIVATE_WITH_PATH ${LIBS_PRIVATE} )
...
@derselbst
Copy link
Member

Ok, understood. I tried to reproduce the problem locally, but couldn't trigger a linker error. I've tried setting up an extra step in the linux CI pipeline (on this branch) but this succeeds as well for both, gcc and clang. About which functions is the linker complaining in your case?

Apart from that I thought through this problem and agree that introducing (yet another) list seems to be the best idea. If possible, I would like to see a CI test failing before we fix this. So if you have any suggestions for triggering that, you're welcome.

@derselbst derselbst added this to the 2.3 milestone Mar 19, 2023
@FtZPetruska
Copy link
Contributor Author

Sure!

The issue happens specifically on my macbook since OpenMP is not a compiler built-in for Apple Clang.
Here are the steps I took to reproduce the error:

  • Have libomp installed through Homebrew (brew install libomp)
  • Configure a static build of FluidSynth with OpenMP enabled:
    cmake -S. -Bbuild -DCMAKE_INSTALL_PREFIX=/opt/local -Denable-openmp=ON -Denable-framework=OFF -DBUILD_SHARED_LIBS=OFF
  • Build and install (cmake --build build && sudo cmake --install build)
  • Try to build an example program using pkg-config:
    clang example.c $(pkg-config --cflags --static --libs fluidsynth)
  • Linking fails with the following error:
Undefined symbols for architecture arm64:
  "___kmpc_barrier", referenced from:
      _.omp_outlined..11 in libfluidsynth.a(fluid_defsfont.c.o)
      _.omp_outlined. in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_critical", referenced from:
      _.omp_task_entry. in libfluidsynth.a(fluid_defsfont.c.o)
      _.omp_task_entry..10 in libfluidsynth.a(fluid_defsfont.c.o)
  "___kmpc_end_critical", referenced from:
      _.omp_task_entry. in libfluidsynth.a(fluid_defsfont.c.o)
      _.omp_task_entry..10 in libfluidsynth.a(fluid_defsfont.c.o)
  "___kmpc_end_single", referenced from:
      _.omp_outlined..11 in libfluidsynth.a(fluid_defsfont.c.o)
  "___kmpc_for_static_fini", referenced from:
      _.omp_outlined. in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_for_static_init_4", referenced from:
      _.omp_outlined. in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_fork_call", referenced from:
      _fluid_defsfont_load_all_sampledata in libfluidsynth.a(fluid_defsfont.c.o)
      _fluid_rvoice_mixer_render in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_global_thread_num", referenced from:
      _fluid_rvoice_mixer_render in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_omp_task", referenced from:
      _.omp_outlined..11 in libfluidsynth.a(fluid_defsfont.c.o)
  "___kmpc_omp_task_alloc", referenced from:
      _.omp_outlined..11 in libfluidsynth.a(fluid_defsfont.c.o)
  "___kmpc_push_num_threads", referenced from:
      _fluid_rvoice_mixer_render in libfluidsynth.a(fluid_rvoice_mixer.c.o)
  "___kmpc_single", referenced from:
      _.omp_outlined..11 in libfluidsynth.a(fluid_defsfont.c.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This also made me notice another issue. The CMake scripts expect that if readline is found that it has a pkg-config file, but on macOS, the system ships with readline but does not provide a pc file for it:

Readline_LIBRARY:FILEPATH=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/lib/libreadline.tbd
PC_READLINE_FOUND:INTERNAL=

Homebrew's version of readline is not added to the usual search paths for this very reason:

==> Caveats
readline is keg-only, which means it was not symlinked into /opt/homebrew,
because macOS provides BSD libedit.

For compilers to find readline you may need to set:
  export LDFLAGS="-L/opt/homebrew/opt/readline/lib"
  export CPPFLAGS="-I/opt/homebrew/opt/readline/include"

For pkg-config to find readline you may need to set:
  export PKG_CONFIG_PATH="/opt/homebrew/opt/readline/lib/pkgconfig"

This leads to the following issue when calling pkg-config:

Package 'readline', required by 'fluidsynth', not found

@derselbst
Copy link
Member

Ok, I finally got the omp issue reproduced for both, MacOS and Linux, see the above PR. I got confused myself - ofc. the test program has to call a fluidsynth-function that is acually OMP parallelized. As we're linking statically, everything else will get dropped and therefore I never saw the omp related linking errors you see.

And I had a hard time convincing cmake on MacOS Azure Pipeline to actually find and use OpenMP. They're running the latest version of cmake 3.26 which is no longer creating a CMakeError.log. Took some time until I found out it's now written to CMakeConfigureLog.yaml.

Anyway, I would probably merge the linked PR today. But I'm afraid I won't have time to actually implement the fix for openmp as you suggested. It will probably take me until next weekend to do so. Perhaps you might be faster in the meantime.

Regarding the readline issue: I have now worked it around by setting the PKG_CONFIG_PATH variable in the pipeline as suggested (we have been already doing this for libffi). A possible solution could be that fluidsynth only adds the readline component to PC_REQUIRES_PRIV if PC_READLINE_VERSION is set to something (i.e. if a readline.pc file acutally exists). If it doesn't exist, just append it to the new PC_LIBS_PRIV.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants