[OpenMP][libomptarget] Build plugins-nextgen/common/PluginInterface with protected visibility

[kevinsala]

Summary

This commit sets the default visibility of PluginInterface's symbols (in nextgen plugins) as protected. This prevents symbols from a plugin library to be preempted by another plugin library's symbol. It applies the same fix introduced by D136365.

Issue reported by @ggeorgakoudis.

[jhuber6]

LG

openmp/libomptarget/plugins-nextgen/common/PluginInterface/CMakeLists.txt

22–25

nit. I'd prefer it in a single target.

[kevinsala]

@jhuber6 Shouldn't we compile the elf_common "library" with protected visibility too?

[jhuber6]

In D138002#3926596, @kevinsala wrote:

@jhuber6 Shouldn't we compile the elf_common "library" with protected visibility too?

It should be fine since everything links from a single library. The previous problem was because we had multiple libraries defining the same functions in the plugins.

[jhuber6]

In D138002#3926598, @jhuber6 wrote:

In D138002#3926596, @kevinsala wrote:

@jhuber6 Shouldn't we compile the elf_common "library" with protected visibility too?

It should be fine since everything links from a single library. The previous problem was because we had multiple libraries defining the same functions in the plugins.

Double checked, it should be an object library so it doesn't export anything on the library level, then the version script will ensure that we only export the plugin interfaces.

[kevinsala]

In D138002#3926600, @jhuber6 wrote:

In D138002#3926598, @jhuber6 wrote:

In D138002#3926596, @kevinsala wrote:

@jhuber6 Shouldn't we compile the elf_common "library" with protected visibility too?

It should be fine since everything links from a single library. The previous problem was because we had multiple libraries defining the same functions in the plugins.

Double checked, it should be an object library so it doesn't export anything on the library level, then the version script will ensure that we only export the plugin interfaces.

Thank you for checking it!

[kevinsala]

Updated to use set_target_properties instead of set_property.

@jhuber6 I'd appreciate if you could commit the patch for me. Thank you in advance.

[tianshilei1992]

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

[jhuber6]

In D138002#3969000, @tianshilei1992 wrote:

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

Can you check if that symbol is actually protected in the .so you're using?

[tianshilei1992]

In D138002#3969003, @jhuber6 wrote:

In D138002#3969000, @tianshilei1992 wrote:

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

Can you check if that symbol is actually protected in the .so you're using?

They are not protected.

➜  readelf -s /home/ac.shilei.tian/Documents/build/openmp/debug/lib/libomptarget.rtl.x86_64.nextgen.so | rg is_va
 77783: 000000000046b7b0   472 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]
 77866: 000000000046b710   157 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]

[jhuber6]

In D138002#3969018, @tianshilei1992 wrote:

In D138002#3969003, @jhuber6 wrote:

In D138002#3969000, @tianshilei1992 wrote:

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

Can you check if that symbol is actually protected in the .so you're using?

They are not protected.

➜  readelf -s /home/ac.shilei.tian/Documents/build/openmp/debug/lib/libomptarget.rtl.x86_64.nextgen.so | rg is_va
 77783: 000000000046b7b0   472 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]
 77866: 000000000046b710   157 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]

That's weird, the CMake should have the visibility preset for all of those libraries as far as I can tell. Think you could check the flags it's using to build the library?

[kevinsala]

In D138002#3969020, @jhuber6 wrote:

In D138002#3969018, @tianshilei1992 wrote:

In D138002#3969003, @jhuber6 wrote:

In D138002#3969000, @tianshilei1992 wrote:

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

Can you check if that symbol is actually protected in the .so you're using?

They are not protected.

➜  readelf -s /home/ac.shilei.tian/Documents/build/openmp/debug/lib/libomptarget.rtl.x86_64.nextgen.so | rg is_va
 77783: 000000000046b7b0   472 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]
 77866: 000000000046b710   157 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]

That's weird, the CMake should have the visibility preset for all of those libraries as far as I can tell. Think you could check the flags it's using to build the library?

@tianshilei1992 Could you attach the call stack trace too? Thanks!

[jhuber6]

In D138002#3969018, @tianshilei1992 wrote:

In D138002#3969003, @jhuber6 wrote:

In D138002#3969000, @tianshilei1992 wrote:

Currently I'm still encountering the issue that, the function call to __tgt_rtl_is_valid_binary in __tgt_rtl_is_valid_binary_info jumps to another plugin.

Can you check if that symbol is actually protected in the .so you're using?

They are not protected.

➜  readelf -s /home/ac.shilei.tian/Documents/build/openmp/debug/lib/libomptarget.rtl.x86_64.nextgen.so | rg is_va
 77783: 000000000046b7b0   472 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]
 77866: 000000000046b710   157 FUNC    GLOBAL DEFAULT   12 __tgt_rtl_is_val[...]

Shows up protected for me

> readelf -Ws ../../clang/lib/libomptarget.rtl.x86_64.nextgen.so | grep is_valid                                                                                              
   104: 000000000000d850    92 FUNC    GLOBAL PROTECTED   15 __tgt_rtl_is_valid_binary                                                                                                                                 
   113: 00000000000128f0   842 FUNC    GLOBAL PROTECTED   15 __tgt_rtl_is_valid_binary_info

openmp/libomptarget/plugins-nextgen/common/PluginInterface/CMakeLists.txt

 add_library(PluginInterface OBJECT PluginInterface.cpp GlobalHandler.cpp)
 
 # Define the TARGET_NAME.
 add_definitions("-DTARGET_NAME=PluginInterface")
 
 # Define the DEBUG_PREFIX.
 add_definitions(-DDEBUG_PREFIX="PluginInterface")
 
-set_property(TARGET PluginInterface PROPERTY POSITION_INDEPENDENT_CODE ON)
+set_target_properties(PluginInterface PROPERTIES
+  POSITION_INDEPENDENT_CODE ON
+  CXX_VISIBILITY_PRESET protected)
 llvm_update_compile_flags(PluginInterface)
 set(LINK_LLVM_LIBS LLVMSupport)
 if (LLVM_LINK_LLVM_DYLIB)
   set(LINK_LLVM_LIBS LLVM)
 endif()
 target_link_libraries(PluginInterface INTERFACE ${LINK_LLVM_LIBS} PRIVATE elf_common MemoryManager)
 add_dependencies(PluginInterface ${LINK_LLVM_LIBS})
 
 target_include_directories(PluginInterface INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
 target_include_directories(PluginInterface PRIVATE ${LIBOMPTARGET_INCLUDE_DIR})