i#5570: Make nohide the default for static libs. (#5581)

Renames dynamorio_static_nohide to dynamorio_static and deprecates
the version where we hide DR symbols using hide_symbols.

Removes tobuild_static_nohide_api, since now all static libs use
the nohide variant.

Documents hide_symbols as unsafe because it may lead to
confusing linking behavior.

Issue: #5570, #3348

core/CMakeLists.txt

@@ -775,6 +775,11 @@ endif (UNIX)
 # DynamoRIO static core library
 
 # hide_symbols is only honored for UNIX.
+# i#3348, i#5574: Note that hide_symbols is not safe. It leads to confusing
+# behavior where different libraries may see different definitions of exported
+# symbols, without obeying WEAK. E.g. the weakly defined routines in drlibc will
+# not be overriden by their respective strong definitions in core DR, for the
+# callsites in drlibc itself.
 function (configure_static_core_lib name hide_symbols)
   add_library(${name} STATIC
     ${CORE_SRCS} ${ARCH_SRCS} ${OS_SRCS})
@@ -841,35 +846,29 @@ endfunction()
 
 # XXX i#1997: not fully supported on Mac yet
 if (NOT APPLE)
-  configure_static_core_lib(dynamorio_static ON)
-  if (UNIX)
-    # Create a version without hidden symbols to support that model.
-    # The symbol hiding step is complex and for some toolchains it is simpler
-    # to avoid it and count on symbol names preventing collisions (i#3348).
-    # XXX i#5570: Cleanup: rename the following to make it clear that it's the
-    # default variant. Also evaluate which of the static_nohide_api tests we
-    # do not need anymore.
-    configure_static_core_lib(dynamorio_static_nohide OFF)
-
-    if (LINUX AND READELF_EXECUTABLE)
-      # We already located readelf above for the shared lib.
-      # We also already created a loc.cmake file in configure_static_core_lib.
-      set(locvar_name dynamorio_static_nohide_loc)
-      add_custom_command(TARGET dynamorio_static_nohide
-        POST_BUILD
-        COMMAND ${CMAKE_COMMAND}
-        ARGS -D lib_fileloc=${CMAKE_CURRENT_BINARY_DIR}/${locvar_name}
-             -D READELF_EXECUTABLE=${READELF_EXECUTABLE}
-             -D CMAKE_SYSTEM_PROCESSOR=${CMAKE_SYSTEM_PROCESSOR}
-             -D X86=${X86}
-             -D X64=${X64}
-             -D DEBUG=${DEBUG}
-             -P ${CMAKE_CURRENT_SOURCE_DIR}/CMake_symbol_check.cmake
-        VERBATIM
-        )
-    endif ()
+  # We do not hide symbols while building dynamorio_static, because it is unsafe
+  # to do so. See comment at configure_static_core_lib for more details.
+  # There are downsides to not hiding symbols that we have to live with,
+  # including pushing our many global symbols into the app namespace. We have
+  # renamed some of the ones that are more likely to collide in #3348.
+  configure_static_core_lib(dynamorio_static OFF)
+  if (LINUX AND READELF_EXECUTABLE)
+    # We already located readelf above for the shared lib.
+    # We also already created a loc.cmake file in configure_static_core_lib.
+    set(locvar_name dynamorio_static_loc)
+    add_custom_command(TARGET dynamorio_static
+      POST_BUILD
+      COMMAND ${CMAKE_COMMAND}
+      ARGS -D lib_fileloc=${CMAKE_CURRENT_BINARY_DIR}/${locvar_name}
+           -D READELF_EXECUTABLE=${READELF_EXECUTABLE}
+           -D CMAKE_SYSTEM_PROCESSOR=${CMAKE_SYSTEM_PROCESSOR}
+           -D X86=${X86}
+           -D X64=${X64}
+           -D DEBUG=${DEBUG}
+           -P ${CMAKE_CURRENT_SOURCE_DIR}/CMake_symbol_check.cmake
+      VERBATIM
+      )
   endif ()
-
 endif (NOT APPLE)
 
 ###########################################################################

make/DynamoRIOConfig.cmake.in

@@ -1109,18 +1109,7 @@ function (configure_DynamoRIO_static target)
   # We need libc before the ntdll_imports pulled in for DR.
   _DR_get_static_libc_list(static_libc)
   target_link_libraries(${target} ${static_libc})
-  # For the weakly linked routines defined in drlibc, we want to use DR's
-  # definitions when available. So we use dynamorio_static_nohide, which does
-  # not hide its symbols, for building static binaries. There are downsides that
-  # we have to live with, including pushing our many global symbols into the app
-  # namespace. We have renamed some of the ones that are more likely to collide
-  # in #3348.
-  if (UNIX)
-    target_link_libraries(${target} dynamorio_static_nohide)
-  else (UNIX)
-    # Non-Unix does not build the nohide version anyway.
-    target_link_libraries(${target} dynamorio_static)
-  endif (UNIX)
+  target_link_libraries(${target} dynamorio_static)
   if (UNIX AND NOT APPLE) # --as-needed not supported by Mac ld
     # i#2070: avoid pulling libdynamorio.so
     # Assuming LINK_FLAGS goes before target_link_libraries.

suite/tests/CMakeLists.txt

@@ -874,22 +874,6 @@ macro(tobuild_api test source dr_ops exe_ops use_decoder use_static_DR pass_opts
     ${pass_opts_via_env})
 endmacro(tobuild_api)
 
-# tobuild_static_nohide_api builds a static API test which uses the
-# dynamorio_static_nohide as a base library. It is a macro so the PARENT_SCOPE
-# in torunonly_api will reach caller of this.
-# XXX: Do we want to support a nicer API for 'nohide' than assuming that
-# add_api_exe adds dynamorio_static and replacing it manually with
-# dynamorio_static_nohide? Punting until there are more uses: it's a lot of
-# plumbing for no clear gain.
-macro(tobuild_static_nohide_api test source dr_ops exe_ops use_decoder)
-  add_api_exe(${test} ${source} ${use_decoder} ON)
-  torunonly_api(${test} OFF ${source} "${dr_ops}" "${exe_ops}" ${use_decoder} OFF OFF)
-  get_target_property(lib_list ${test} LINK_LIBRARIES)
-  list(REMOVE_ITEM lib_list dynamorio_static)
-  set_property(TARGET ${test} PROPERTY LINK_LIBRARIES ${lib_list} )
-  target_link_libraries(${test} dynamorio_static_nohide)
-endmacro(tobuild_static_nohide_api)
-
 # run that uses a different build
 function(torunonly test realtest source dr_ops exe_ops)
   set(testlist_normal ${testlist_normal} ${test} PARENT_SCOPE)
@@ -3092,27 +3076,25 @@ if (NOT ANDROID AND NOT APPLE)
     # i#3348: Ensure that DR without local symbols hidden is linkable and
     # avoids conflicts (though CMake_symbol_check does most of the conflict
     # checking).
-    tobuild_static_nohide_api(api.static_symbols api/static_symbols.c "" "" OFF)
+
+    tobuild_api(api.static_symbols api/static_symbols.c "" "" OFF ON OFF)
     append_link_flags(api.static_symbols "-Wl,--warn-common -Wl,--fatal-warnings")
 
     # To ensure that the dynamorio_so_start/_end linker variables are actually
     # used in static binaries, we have two tests which run the same "map mixup"
     # logic: one which exits successfully because we provide the linker
     # variables and another which exits with an assertion because we don't.
-    # Both versions utilize the dynamorio_static_nohide as a base because
-    # the variables in dynamorio_static have already been resolved as a part of
-    # the symbol hiding step.
     set(api.static_maps_mixup_yesvars_expectbase
        "static_maps_mixup_yesvars")
-    tobuild_static_nohide_api(api.static_maps_mixup_yesvars
-       api/static_maps_mixup.c "" "" OFF)
+    tobuild_api(api.static_maps_mixup_yesvars
+       api/static_maps_mixup.c "" "" OFF ON OFF)
     append_link_flags(api.static_maps_mixup_yesvars
         "-Wl,--defsym,dynamorio_so_start=__executable_start -Wl,--defsym,dynamorio_so_end=end")
 
     set(api.static_maps_mixup_novars_FLAKY_expectbase
        "static_maps_mixup_novars")
-    tobuild_static_nohide_api(api.static_maps_mixup_novars_FLAKY
-       api/static_maps_mixup.c "" "" OFF)
+    tobuild_api(api.static_maps_mixup_novars_FLAKY
+       api/static_maps_mixup.c "" "" OFF ON OFF)
   endif ()
 
   if (NOT WIN32) # TODO i#4349: Fix re-attach issues to enable.