[NFC] Rename LLVM_NO_DEAD_STRIP

[daltenty]

Summary

The variable LLVM_NO_DEAD_STRIP is set in LLVM cmake files when building executables that might make use of plugins .The name of the variable does not convey the actual intended usage (i.e. for use with tools that have plugins), just what the eventual effect of setting in on some (i.e. not garbage collecting unused symbols).

This patch renames it to LLVM_SUPPORT_PLUGINS to convey the intended usage, which will allow subsequent patches to add behavior to support that in different ways without confusion about whether it will do on, for example, non-gnu platforms.

[stevewan]

The variable LLVM_NO_DEAD_STRIP also existed in the following clang code, do we also need to rename those?

./clang/tools//driver/CMakeLists.txt:23:# LLVM_NO_DEAD_STRIP.
./clang/tools//driver/CMakeLists.txt:25:  set(LLVM_NO_DEAD_STRIP 1)

[daltenty]

Address comments round 1

[stevewan]

LGTM

[beanz]

LLVM_NO_DEAD_STRIP isn't actually useful only for when plugins are enabled. It is also useful in a lot of contexts around building JIT host processes. The advantage of the old name was it specified exactly what it did (i.e. not dead strip code).

It seems to me that maybe a more appropriate approach is that LLVM_SUPPORT_PLUGINS implies LLVM_NO_DEAD_STRIP, rather than conflating the two options.

[lhames]

In D69356#1726074, @beanz wrote:

... It seems to me that maybe a more appropriate approach is that LLVM_SUPPORT_PLUGINS implies LLVM_NO_DEAD_STRIP, rather than conflating the two options.

Yep — there are use-cases for no-dead-strip that aren’t plugins. I’m not sure this rename helps. I think the .*PLUGINS.* options need more consideration, and that LLVM_NO_DEAD_STRIP should be reinstated.

[hubert.reinterpretcast]

In D69356#1726121, @lhames wrote:

In D69356#1726074, @beanz wrote:

... It seems to me that maybe a more appropriate approach is that LLVM_SUPPORT_PLUGINS implies LLVM_NO_DEAD_STRIP, rather than conflating the two options.

Yep — there are use-cases for no-dead-strip that aren’t plugins. I’m not sure this rename helps. I think the .*PLUGINS.* options need more consideration, and that LLVM_NO_DEAD_STRIP should be reinstated.

Is there some documentation indicating these other use cases? The current instances are consistent with plug-in support. The "no dead strip" semantic is wrong and harmful for plug-in support on some platforms, so the suggestion to imply "no dead strip" when plug-in support is requested might not be advisable.

[beanz]

In D69356#1726336, @hubert.reinterpretcast wrote:

Is there some documentation indicating these other use cases?

If you have JIT'd code that needs to link against functions exposed in the process running the JIT you can't dead strip at link time otherwise those functions will be removed.

The current instances are consistent with plug-in support. The "no dead strip" semantic is wrong and harmful for plug-in support on some platforms, so the suggestion to imply "no dead strip" when plug-in support is requested might not be advisable.

Can you please explain how the "no dead strip" semantic is harmful for plug-in support, and how implying no dead strip for plugins is any different from the current implementation. This patch does not change behavior, it just makes the option name more confusing my linking disabling dead stripping to plugins. If what you say is correct that "no dead strip" is harmful to plugins on some platforms it seems like this rename was a step in the direction of more confusion.

[hubert.reinterpretcast]

In D69356#1726354, @beanz wrote:

Can you please explain how the "no dead strip" semantic is harmful for plug-in support, and how implying no dead strip for plugins is any different from the current implementation.

For your first question: AIX performs garbage collection by default. Disabling garbage collection does not work (regardless of plug-in support). The system libraries contain references to undefined symbols from sections that are normally garbage collected.
For your second question: As the patch author indicated, the change to adjust the behaviour on the affected platform is forthcoming. Your question seems predicated upon that not happening.

This patch does not change behavior, it just makes the option name more confusing my linking disabling dead stripping to plugins.

Just because the current implementation of plug-in support is by disabling dead strip does not make it a great reason for "enabling plug-in support" to be called LLVM_NO_DEAD_STRIP.

If what you say is correct that "no dead strip" is harmful to plugins on some platforms it seems like this rename was a step in the direction of more confusion.

Only when focusing on the implementation and not the intent.

If you believe there should be a way to request "no dead strip" regardless of what the user intended it for (and whether it would work for their intended use case), then I guess we can keep such an option. In such a case, then for some platforms, requesting that an executable be linked such that plug-in support is enabled will imply "no dead strip" (as you suggested). Perhaps the more accurate intent for both the JIT host process and the plug-in support case is that the executable is expected to make symbols available for run-time linking. Maybe LLVM_EXPORT_SYMBOLS_FROM_EXEC is a better name?

[beanz]

In D69356#1726527, @hubert.reinterpretcast wrote:

For your second question: As the patch author indicated, the change to adjust the behaviour on the affected platform is forthcoming. Your question seems predicated upon that not happening.

I'm actually opposed to that happening, on two fronts.
(1) dead stripping support is useful independent of plugins, so it is valuable to have that option be separate
(2) we already have LLVM_ENABLE_PLUGINS why do we also need LLVM_SUPPORT_PLUGINS seems like duplication and bad design.

Just because the current implementation of plug-in support is by disabling dead strip does not make it a great reason for "enabling plug-in support" to be called LLVM_NO_DEAD_STRIP.

LLVM_NO_DEAD_STRIP isn't the implementation of enabling plugin support.

Maybe LLVM_EXPORT_SYMBOLS_FROM_EXEC is a better name?

Not really because exporting symbols and dead stripping aren't actually the same thing. You can dead strip unused symbols *and* export everything else.

I think this change needs to be rolled back, and future changes to the build system should include reviewers with experience maintaining the build system.

[daltenty]

This comment was removed by daltenty.

[daltenty]

I'm actually opposed to that happening, on two fronts.
(2) we already have LLVM_ENABLE_PLUGINS why do we also need LLVM_SUPPORT_PLUGINS seems like duplication and bad design.

As I understand it LLVM_ENABLE_PLUGINS is a user-facing option which indicates whether the user wants to build with plugin support.

The intention of LLVM_SUPPORT_PLUGINS was as an internal option set by tools which may have plugins and need to be built in a specific way (such as avoiding deadstriping) if plugins are enabled.

(1) dead stripping support is useful independent of plugins, so it is valuable to have that option be separate

We could keep LLVM_NO_DEAD_STRIP as is and have it set by LLVM_SUPPORT_PLUGINS when appropriate. That should accommodate both uses.

[beanz]

In D69356#1727451, @daltenty wrote:

The intention of LLVM_SUPPORT_PLUGINS was as an internal option set by tools which may have plugins and need to be built in a specific way (such as avoiding deadstriping) if plugins are enabled.

Non-user facing options shouldn't be exposed this way. LLVM_NO_DEAD_STRIP has been around a long time and predates many of the modern CMake patterns. We should use arguments passed in explicitly to the calling functions rather than setting variables that are passed down.

We could keep LLVM_NO_DEAD_STRIP as is and have it set by LLVM_SUPPORT_PLUGINS when appropriate. That should accommodate both uses.

We should not be adding more variables that are passed around by CMake's scope inheritance. Instead if we need to change this we should do it correctly.

[hubert.reinterpretcast]

In D69356#1727381, @beanz wrote:

(1) dead stripping support is useful independent of plugins, so it is valuable to have that option be separate

Okay.

Just because the current implementation of plug-in support is by disabling dead strip does not make it a great reason for "enabling plug-in support" to be called LLVM_NO_DEAD_STRIP.

LLVM_NO_DEAD_STRIP isn't the implementation of enabling plugin support.

Sure, it can be claimed that it merely has high affinity with the implementation of enabling plugin support in terms of linking in its use prior to the subject patch. Adding LLVM_NO_DEAD_STRIP back is fine if it is used for something else, although it would help people developing patches understand its use cases better if there was more comments and better documentation. The comments about LLVM_NO_DEAD_STRIP that can be seen in the patch context talk a lot about plug-in support, and mentions of other use cases are not really standing out.

[hubert.reinterpretcast]

In D69356#1727484, @beanz wrote:

We should not be adding more variables that are passed around by CMake's scope inheritance. Instead if we need to change this we should do it correctly.

Just to clarify, the actions to take are to restore LLVM_NO_DEAD_STRIP in order to support other uses and then to replace its use as an internal variable? Given that direction, I agree the first course of action is to revert this patch and then to pursue a patch to do the replacement separately.

[daltenty]

In D69356#1727581, @hubert.reinterpretcast wrote:

In D69356#1727484, @beanz wrote:

We should not be adding more variables that are passed around by CMake's scope inheritance. Instead if we need to change this we should do it correctly.

Just to clarify, the actions to take are to restore LLVM_NO_DEAD_STRIP in order to support other uses and then to replace its use as an internal variable? Given that direction, I agree the first course of action is to revert this patch and then to pursue a patch to do the replacement separately.

Based on the discussion, I will revert this change and open a new review introducing SUPPORT_PLUGINS as an option to the add_llvm_tool macro.

[daltenty]

Revised direction in https://reviews.llvm.org/D69638

clang/tools/driver/CMakeLists.txt

set( LLVM_LINK_COMPONENTS
   Support
   TransformUtils
   Vectorize
   )
 
 option(CLANG_PLUGIN_SUPPORT "Build clang with plugin support" ON)
 
 # Support plugins. This must be before add_clang_executable as it reads
-# LLVM_NO_DEAD_STRIP.
+# LLVM_SUPPORT_PLUGINS.
 if(CLANG_PLUGIN_SUPPORT)
-  set(LLVM_NO_DEAD_STRIP 1)
+  set(LLVM_SUPPORT_PLUGINS 1)
 endif()
 
 if(NOT CLANG_BUILT_STANDALONE)
   set(tablegen_deps intrinsics_gen)
 endif()
 
 add_clang_tool(clang
   driver.cpp

llvm/cmake/modules/AddLLVM.cmake

if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
     if(LLVM_LINKER_IS_GOLD)
       # With gold gc-sections is always safe.
       set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                    LINK_FLAGS " -Wl,--gc-sections")
       # Note that there is a bug with -Wl,--icf=safe so it is not safe
       # to enable. See https://sourceware.org/bugzilla/show_bug.cgi?id=17704.
     endif()
 
-    if(NOT LLVM_NO_DEAD_STRIP)
+    if(NOT LLVM_SUPPORT_PLUGINS)
       if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
         # ld64's implementation of -dead_strip breaks tools that use plugins.
         set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                      LINK_FLAGS " -Wl,-dead_strip")
       elseif(${CMAKE_SYSTEM_NAME} MATCHES "SunOS")
         set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                      LINK_FLAGS " -Wl,-z -Wl,discard-unused=sections")
       elseif(NOT WIN32 AND NOT LLVM_LINKER_IS_GOLD AND
              NOT ${CMAKE_SYSTEM_NAME} MATCHES "OpenBSD|AIX")
         # Object files are compiled with -ffunction-data-sections.
         # Versions of bfd ld < 2.23.1 have a bug in --gc-sections that breaks
         # tools that use plugins. Always pass --gc-sections once we require
         # a newer linker.
         set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                      LINK_FLAGS " -Wl,--gc-sections")
       endif()
-    else() #LLVM_NO_DEAD_STRIP
+    else() #LLVM_SUPPORT_PLUGINS
       if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
         set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                      LINK_FLAGS " -Wl,-bnogc")
       endif()
     endif()
   endif()
 endfunction(add_link_opts)
 

llvm/cmake/modules/HandleLLVMOptions.cmake

if (UNIX AND CMAKE_GENERATOR STREQUAL "Ninja")
   check_linker_flag("-Wl,--color-diagnostics" LINKER_SUPPORTS_COLOR_DIAGNOSTICS)
   append_if(LINKER_SUPPORTS_COLOR_DIAGNOSTICS "-Wl,--color-diagnostics"
     CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
 endif()
 
 # Add flags for add_dead_strip().
 # FIXME: With MSVS, consider compiling with /Gy and linking with /OPT:REF?
 # But MinSizeRel seems to add that automatically, so maybe disable these
-# flags instead if LLVM_NO_DEAD_STRIP is set.
+# flags instead if LLVM_SUPPORT_PLUGINS is set.
 if(NOT CYGWIN AND NOT WIN32)
   if(NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin" AND
      NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
     check_c_compiler_flag("-Werror -fno-function-sections" C_SUPPORTS_FNO_FUNCTION_SECTIONS)
     if (C_SUPPORTS_FNO_FUNCTION_SECTIONS)
       # Don't add -ffunction-section if it can be disabled with -fno-function-sections.
       # Doing so will break sanitizers.
       add_flag_if_supported("-ffunction-sections" FFUNCTION_SECTIONS)

llvm/tools/bugpoint/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   ScalarOpts
   Support
   Target
   TransformUtils
   Vectorize
   )
 
 # Support plugins.
-set(LLVM_NO_DEAD_STRIP 1)
+set(LLVM_SUPPORT_PLUGINS 1)
 
 add_llvm_tool(bugpoint
   BugDriver.cpp
   CrashDebugger.cpp
   ExecutionDriver.cpp
   ExtractFunction.cpp
   FindBugs.cpp
   Miscompilation.cpp

llvm/tools/llc/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   SelectionDAG
   Support
   Target
   TransformUtils
   Vectorize
   )
 
 # Support plugins.
-set(LLVM_NO_DEAD_STRIP 1)
+set(LLVM_SUPPORT_PLUGINS 1)
 
 add_llvm_tool(llc
   llc.cpp
 
   DEPENDS
   intrinsics_gen
   )
 export_executable_symbols(llc)

llvm/tools/opt/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   Support
   Target
   TransformUtils
   Vectorize
   Passes
   )
 
 # Support plugins.
-set(LLVM_NO_DEAD_STRIP 1)
+set(LLVM_SUPPORT_PLUGINS 1)
 
 add_llvm_tool(opt
   AnalysisWrappers.cpp
   BreakpointPrinter.cpp
   Debugify.cpp
   GraphPrinters.cpp
   NewPMDriver.cpp
   PassPrinters.cpp