[Libomptarget] Pass '-Werror=global-constructors' to the libomptarget build #88531
Conversation
jhuber6
requested review from
carlobertolli,
jdoerfert,
jplehr,
Meinersbur,
ronlieb,
saiislam and
shiltian
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -54,7 +54,7 @@ struct RecordReplayTy { | |||
| size_t MemorySize = 0; | |||
| size_t TotalSize = 0; | |||
| GenericDeviceTy *Device = nullptr; | |||
| std::mutex AllocationLock; | |||
| std::mutex AllocationLock{}; | |||
There was a problem hiding this comment.
That was me trying to make it trivially constructable, but then realized the issue is the vector.
| @@ -362,7 +362,10 @@ struct RecordReplayTy { | |||
| } | |||
| }; | |||
|
|
|||
| static RecordReplayTy RecordReplay; | |||
| RecordReplayTy &getRecordReplay() { | |||
There was a problem hiding this comment.
Add some comment about the singleton pattern and whether it is thread-safe ("magic static")?
There was a problem hiding this comment.
I think I just need to move this into the plugin initialization code. @jdoerfert could we just move this into the plugin instead? I'm assuming we don't need this to actually be global and it would make the lifetime much nicer.
| if (getRecordReplay().isRecording()) { | ||
| getRecordReplay().saveImage(getName(), getImage()); | ||
| getRecordReplay().saveKernelInput(getName(), getImage()); | ||
| getRecordReplay().saveKernelDescr(getName(), Ptrs.data(), |
There was a problem hiding this comment.
Each call of getRecordReplay involves a lock. It should be called just once.
There was a problem hiding this comment.
Yeah, I should probably make a patch before this that just initializes it with the plugin initialization.
| @@ -23,6 +23,9 @@ | |||
|
|
|||
| using namespace llvm; | |||
|
|
|||
| #pragma GCC diagnostic push | |||
| #pragma GCC diagnostic ignored "-Wglobal-constructors" | |||
There was a problem hiding this comment.
Does Clang and other non-GCC compilers honor this as well?
There was a problem hiding this comment.
They don't honor -Werror=global-constructors so it wouldn't matter, and unrecognized pragmas are silently ignored AFAIK.
There was a problem hiding this comment.
Clang does warn about unknown pragmas with -Wall, but seems to actually support '#pragma GCC diagnostic: https://godbolt.org/z/WfTrMGr5d
There was a problem hiding this comment.
A better solution in the future would probably be to just make this tool not link directly with the plugin object.
… build Summary: A runtime library should not have global constructors. This has caused many issues in the past so we would make them a hard error if they show up. This required rewriting the RecordReplay implementation because it uses a SmallVector internally which can't be made constexpr.
|
@llvm/pr-subscribers-offload Author: Joseph Huber (jhuber6) ChangesSummary: Full diff: https://github.com/llvm/llvm-project/pull/88531.diff 2 Files Affected:
diff --git a/offload/CMakeLists.txt b/offload/CMakeLists.txt
index 1d8cab240924e..e2a4c2597636e 100644
--- a/offload/CMakeLists.txt
+++ b/offload/CMakeLists.txt
@@ -136,6 +136,8 @@ include(LibomptargetGetDependencies)
# Set up testing infrastructure.
include(OpenMPTesting)
+check_cxx_compiler_flag(-Werror=global-constructors OFFLOAD_HAVE_WERROR_CTOR)
+
# LLVM source tree is required at build time for libomptarget
if (NOT LIBOMPTARGET_LLVM_INCLUDE_DIRS)
message(FATAL_ERROR "Missing definition for LIBOMPTARGET_LLVM_INCLUDE_DIRS")
@@ -207,6 +209,9 @@ set(offload_compile_flags -fno-exceptions)
if(NOT LLVM_ENABLE_RTTI)
set(offload_compile_flags ${offload_compile_flags} -fno-rtti)
endif()
+if(OFFLOAD_HAVE_WERROR_CTOR)
+ list(APPEND offload_compile_flags -Werror=global-constructors)
+endif()
# TODO: Consider enabling LTO by default if supported.
# https://cmake.org/cmake/help/latest/module/CheckIPOSupported.html can be used
diff --git a/offload/src/CMakeLists.txt b/offload/src/CMakeLists.txt
index b474f29ea0be2..c5338130297fe 100644
--- a/offload/src/CMakeLists.txt
+++ b/offload/src/CMakeLists.txt
@@ -69,8 +69,8 @@ foreach(plugin IN LISTS LIBOMPTARGET_PLUGINS_TO_BUILD)
target_link_libraries(omptarget PRIVATE omptarget.rtl.${plugin})
endforeach()
-target_compile_options(omptarget PUBLIC ${offload_compile_flags})
-target_link_options(omptarget PUBLIC ${offload_link_flags})
+target_compile_options(omptarget PRIVATE ${offload_compile_flags})
+target_link_options(omptarget PRIVATE ${offload_link_flags})
# libomptarget.so needs to be aware of where the plugins live as they
# are now separated in the build directory.
|
|
Finally got rid of the global constructors in source. |
Summary:
A runtime library should not have global constructors. Everything is now expected to go through the init methods. This patch ensures that global constructors will not accidentally be introduced.