Page MenuHomePhabricator

[NFC] Rename LLVM_NO_DEAD_STRIP
ClosedPublic

Authored by daltenty on Oct 23 2019, 1:51 PM.

Details

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.

Diff Detail

Event Timeline

daltenty created this revision.Oct 23 2019, 1:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2019, 1:51 PM
daltenty retitled this revision from Rename LLVM_NO_DEAD_STRIP to [NFC] Rename LLVM_NO_DEAD_STRIP.Oct 23 2019, 2:05 PM
daltenty edited the summary of this revision. (Show Details)

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 updated this revision to Diff 226319.Oct 24 2019, 1:24 PM

Address comments round 1

Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 1:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision is now accepted and ready to land.Oct 24 2019, 1:37 PM
This revision was automatically updated to reflect the committed changes.
beanz added a subscriber: beanz.Oct 29 2019, 2:31 PM

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.

beanz removed a subscriber: beanz.Oct 29 2019, 2:48 PM

... 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.

... 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 added a comment.Oct 29 2019, 5:27 PM

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.

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?

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.

This comment was removed by 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.

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.

(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.

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.

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.