Page MenuHomePhabricator

[OMPT] Provide initialization for Mac OS X
ClosedPublic

Authored by Hahnfeld on Nov 8 2017, 7:21 AM.

Details

Summary

Traditionally, the library had a weak symbol for ompt_start_tool()
that served as fallback and disabled OMPT if called. Tools could
provide their own version and replace the default implementation
to register callbacks and lookup functions. This mechanism has
worked reasonably well on Linux systems where this interface was
initially developed.

On Darwin / Mac OS X the situation is a bit more complicated and
the weak symbol doesn't work out-of-the-box. In my tests, the
library with the tool needed to link against the OpenMP runtime
to make the process work. This would effectively mean that a tool
needed to choose a runtime library whereas one design goal of the
interface was to allow tools that are agnostic of the runtime.

The solution is to use dlsym() with the argument RTLD_DEFAULT so
that static implementations of ompt_start_tool() are found in the
main executable. This works because the linker on Mac OS X includes
all symbols of an executable in the global symbol table by default.
To use the same code path on Linux, the application would need to
be built with -Wl,--export-dynamic. To avoid this restriction, we
continue to use weak symbols on Linux systems as before.

Finally this patch extends the existing test to cover all possible
ways of initializing the tool as described by the standard. It
also fixes ompt_finalize() to not call omp_get_thread_num() when
the library is shut down which resulted in hangs on Darwin.
The changes have been tested on Linux to make sure that it passes
the current tests as well as the newly extended one.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Nov 8 2017, 7:21 AM
omalyshe added inline comments.Nov 8 2017, 8:09 AM
runtime/cmake/config-ix.cmake
241 ↗(On Diff #122092)

WIN32 AND LIBOMP_HAVE_PSAPI

Hahnfeld updated this revision to Diff 122098.Nov 8 2017, 8:15 AM
Hahnfeld marked an inline comment as done.

Fix typo in config-ix.cmake

runtime/cmake/config-ix.cmake
241 ↗(On Diff #122092)

Oops, right you are. Thanks CMake for telling me...

hbae added a subscriber: hbae.Nov 8 2017, 9:42 AM
hbae added inline comments.
runtime/src/ompt-general.cpp
124 ↗(On Diff #122098)

I think this code now requires LD_DYNAMIC_WEAK to be set in order to override weak symbols (e.g., when libomp.so is linked before tool library).
We used RTLD_NEXT to avoid this, by making the fallback ompt_start_tool search for the next available symbol.

Hahnfeld updated this revision to Diff 122122.Nov 8 2017, 11:14 AM
Hahnfeld added a reviewer: hbae.

Readd dlsym() to cover cases where the runtime is linked before the tool.

runtime/src/ompt-general.cpp
124 ↗(On Diff #122098)

Thanks, that was the test case and explanation that I was missing. I have readded the code with a comment describing the situation and added a test case for this.

Note that KMP_DYNAMIC_LIB is always true and we require OMPT_HAVE_WEAK_ATTRIBUTE for Unix systems.

hbae edited edge metadata.Nov 8 2017, 2:15 PM

LGTM.

LGTM

We might want to add another RUN-line for 3, where the first tool returns NULL for ompt_start_tool and only the second tool in the list is initialized.
We can add this test in a later patch

We might want to add another RUN-line for 3, where the first tool returns NULL for ompt_start_tool and only the second tool in the list is initialized.
We can add this test in a later patch

And another tool that doesn't have ompt_start_tool at all. I will do that (in another patch) because I think that these libraries should be dlclosed if not found suitable.

protze.joachim accepted this revision.Nov 10 2017, 10:32 AM
This revision is now accepted and ready to land.Nov 10 2017, 10:32 AM
This revision was automatically updated to reflect the committed changes.