Page MenuHomePhabricator

[libomptarget] Introduce LIBOMPTARGET_ENABLE_DEBUG cmake option.
ClosedPublic

Authored by Meinersbur on Dec 20 2018, 1:06 PM.

Details

Summary

At the moment, support for runtime debug output using the OMPTARGET_DEBUG=1 environment variable is only available with CMAKE_BUILD_TYPE=Debug builds. The patch allows setting it independently using the LIBOMPTARGET_ENABLE_DEBUG option, which is enabled by default depending on CMAKE_BUILD_TYPE. That is, unless this option is set explicitly, nothing changes. This is the same mechanism used by LLVM for LLVM_ENABLE_ASSERTIONS.

This patch also remove adding -g -O0 in debug builds, it should be handled by cmake.

Idea by @hfinkel

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur created this revision.Dec 20 2018, 1:06 PM
lildmh added a subscriber: lildmh.Dec 21 2018, 1:25 PM

I'm not sure what we are trying to do here. OMPTARGET_DEBUG is not an environment variable, it's a preprocessor definition. It is used by libomptarget/include/omptarget.h to include all necessary header files and define the DEBUGP macro appropriately, which in turn is used by libomptarget/src/private.h to define the DP macro used throughout the library.

The env var you are referring to is LIBOMPTARGET_DEBUG and it switches debug output on and off, provided that the library has been compiled with -DOMPTARGET_DEBUG so that the appropriate macros have been defined in omptarget.h and private.h.

If you want to make debug output available in release mode via a new LIBOMPTARGET_ENABLE_DEBUG cmake option, then you should add another check like:

if(LIBOMPTARGET_ENABLE_DEBUG)
  add_definitions(-DOMPTARGET_DEBUG)
endif()
libomptarget/CMakeLists.txt
48 ↗(On Diff #179136)

This must stay, it's a preprocessor definition, not an env var.

  • Define OMPTARGET_DEBUG preprocessor symbol if LIBOMPTARGET_ENABLE_DEBUG
  • Fix regression test execution
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 4:40 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript

Sorry for the delay. I missed your reply during the holidays.

Thank you for the hint. Somewhat confusingly LIBOMPTARGET_DEBUG is a preprocessor symbol, a CMake config variable and an environment variable. I fixed the execution of check-libomptarget on my machine (it replaced all commands in RUN with ignored-command) so I can confirm that it works.

  • Undo whitespace change

This revision looks much better. Please have a look at the inline comment.

libomptarget/CMakeLists.txt
48–49 ↗(On Diff #190186)

Why are these lines removed? When building in debug mode we need to have debug symbols (-g) and prevent the compiler from optimizing things away (-O0) in order to be able to use a debugger correctly.

Meinersbur marked an inline comment as done.Mar 24 2019, 1:27 PM
Meinersbur added inline comments.
libomptarget/CMakeLists.txt
48–49 ↗(On Diff #190186)

This is mentioned in the summary.

This patch also remove adding -g -O0 in debug builds, it should be handled by cmake.

With CMAKE_BUILD_TYPE=Debug, CMake will add CMAKE_{C|CXX}_FLAGS_DEBUG to CMAKE_{C|CXX}_FLAGS. CMAKE_{C|CXX}_FLAGS_DEBUG is preinitialized with the compiler's debug options, e.g. -O0 -g for gcc. Adding these flags using add_definitions is either redundant, might result in an error for non-gcc-like command line interfaces, or clashes with user-provided debug flags (such as cmake -DCMAKE_C_FLAGS_DEBUG="-gdwarf=5 -Og").

Unless I am overseeing something, in which case please point me to it.

grokos accepted this revision.Mar 25 2019, 7:38 PM

OK, now it makes sense. I had forgotten what was in the description since the patch was first submitted. :)

Looks good now.

This revision is now accepted and ready to land.Mar 25 2019, 7:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 8:17 AM

@grokos Thank you for the review!