Add option -fkeep-static-consts

[eandrews]

Summary

Currently clang does not emit unused static constants. GCC emits these constants by default when there is no optimization.

GCC 's option -fkeep-static-consts seems to be buggy. I am not sure what exactly it does because default behavior emits unused static constants (i.e. without the use of any option) and this option does not work with optimization. Its negation can be used to not emit the static constants.

In Clang, since default behavior does not keep unused static constants, I implemented -fkeep-static-consts to emit these if required. This could be useful for producing identification strings like SVN identifiers inside the object file even though the string isn't used by the program.

[eandrews]

*ping*

[rnk]

This isn't sufficient, GlobalDCE will remove the internal constant. It's also unlikely that the constant will survive --gc-sections / -fdata-sections. A better solution would be to add a new attribute (__attribute__((nondiscardable))? too close to nodiscard?) that adds the global in question to @llvm.used and excludes it from -fdata-sections.

[eandrews]

Thanks for the review Reid. Sorry for the delay in my response. I was OOO.

I am not sure if a new attribute is necessary. __ attribute __(used) is already supported in Clang. While this attribute can be used to retain static constants, it would require the user to modify the source which may not always be possible/practical. Its also interesting to note that GCC actually retains unused static constants by default. fno-keep-static-consts is used to remove unused static constants in GCC.

[rnk]

OK. My concern is that users need to use __attribute__((used)) or something more robust if they want SVN identifiers to reliably appear in the output. Adding this flag just creates a trap that will fail once they turn on -O2. I'd rather not have it in the interface to avoid that user confusion.

[eandrews]

That makes sense. Is it not possible to implement the required functionality using a flag vs an attribute? In an earlier comment you mentioned adding the global to @llvm.used to prevent GlobalDCE from removing it. Can you not do that when using a flag?

[erichkeane]

OK. My concern is that users need to use attribute((used)) or something more robust if they want SVN identifiers to reliably appear in the output. Adding this flag just creates a trap that will fail once they turn on >>-O2. I'd rather not have it in the interface to avoid that user confusion.

Since the idea here is to have a global option for 'always emit these things', could a viable solution simply add all of these things to @llvm.used. This wouldn't affect the usefulness of __attribute((used)), but could be implemented similarly, right?

[eandrews]

I think we can use CodeGenModule::addUsedGlobal for this purpose. I noticed this is what attribute 'used' calls. I need to look into it though.

[eandrews]

This patch fell through the cracks earlier. I apologize. Based on Reid's and Erich's feedback, I am now adding the variable to @llvm.used. Additionally I modified the if statements to ensure only static variables are considered.

I also compared the IR when using the option -fkeep-static-consts while compiling the program, vs adding attribute((used)) to individual variables inside the program. The generated IR had no difference. I assume this means all required information to prevent the variable from being removed in later stages are now being emitted?

[rnk]

include/clang/Basic/LangOptions.def

311

Let's make this a CodeGenOption, since only CodeGen needs to look at it.

[eandrews]

Based on Reid's feedback, I changed option to CodeGenOption

[eandrews]

include/clang/Basic/LangOptions.def

311

Thanks for the feedback Reid! I've changed it

[rnk]

lgtm!

[eandrews]

In D40925#1206416, @rnk wrote:

lgtm!

Thanks!

[rnk]

My coworker, @zturner, asked if I knew a way to force emit all constants, and I mentioned this flag, but we noticed it doesn't work on cases when the constant is implicitly static, like this:

const int foo = 42;

If I add static to it, it gets emitted with -fkeep-static-consts, but as is, it doesn't get emitted.

Do you think it's worth extending the logic to handle this case? GCC seems to that global foo regardless of the flag setting.

[erichkeane]

In D40925#1276114, @rnk wrote:

My coworker, @zturner, asked if I knew a way to force emit all constants, and I mentioned this flag, but we noticed it doesn't work on cases when the constant is implicitly static, like this:

const int foo = 42;

If I add static to it, it gets emitted with -fkeep-static-consts, but as is, it doesn't get emitted.

Do you think it's worth extending the logic to handle this case? GCC seems to that global foo regardless of the flag setting.

I looked into it real quick, and think that checking storage class instead of duration is incorrect here. See https://reviews.llvm.org/D53718 for the fix @zturner is asking for (I think!).

[eandrews]

I think it makes sense to include this case just to make the flag more complete. Also, we don't really match GCC behavior for this flag. GCC emits all static constants by default (when O0). When optimized, this flag has no effect on GCC. The negation is used to not emit the constants.

Erich has uploaded a possible fix here - https://reviews.llvm.org/D53718

include/clang/Basic/LangOptions.def

ENUM_LANGOPT(ClangABICompat, ClangABI, 4, ClangABI::Latest,
              "with")
 
 COMPATIBLE_VALUE_LANGOPT(FunctionAlignment, 5, 0, "Default alignment for functions")
 
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
 LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
         "unsigned fixed point types having one extra padding bit")
 
+BENIGN_LANGOPT(KeepStaticConsts      , 1, 0, "keep static const variables even if unused")
+
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
 #undef ENUM_LANGOPT
 #undef COMPATIBLE_ENUM_LANGOPT
 #undef BENIGN_ENUM_LANGOPT
 #undef VALUE_LANGOPT
 #undef COMPATIBLE_VALUE_LANGOPT
 #undef BENIGN_VALUE_LANGOPT

include/clang/Driver/Options.td

 def fno_jump_tables : Flag<["-"], "fno-jump-tables">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Do not use jump tables for lowering switches">;
 def fforce_enable_int128 : Flag<["-"], "fforce-enable-int128">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Enable support for int128_t type">;
 def fno_force_enable_int128 : Flag<["-"], "fno-force-enable-int128">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Disable support for int128_t type">;
-
+def fkeep_static_consts : Flag<["-"], "fkeep-static-consts">, Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Keep static const variables even if unused">;
 def ffixed_point : Flag<["-"], "ffixed-point">, Group<f_Group>,
                    Flags<[CC1Option]>, HelpText<"Enable fixed point types">;
 def fno_fixed_point : Flag<["-"], "fno-fixed-point">, Group<f_Group>,
                       HelpText<"Disable fixed point types">;
 
 // Begin sanitizer flags. These should all be core options exposed in all driver
 // modes.
 let Flags = [CC1Option, CoreOption] in {

lib/AST/ASTContext.cpp

bool ASTContext::DeclMustBeEmitted(const Decl *D) {
   // Weak references don't produce any output by themselves.
   if (D->hasAttr<WeakRefAttr>())
     return false;
 
   // Aliases and used decls are required.
   if (D->hasAttr<AliasAttr>() || D->hasAttr<UsedAttr>())
     return true;
 
+  // Emit static constants even if they are not used if KeepStaticConsts is set.
+  if (LangOpts.KeepStaticConsts) {
+    const auto *VD = dyn_cast<VarDecl>(D);
+    if (VD && VD->getType().isConstQualified() &&
+        VD->getStorageClass() == SC_Static)
+      return true;
+  }
+
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     // Forward declarations aren't required.
     if (!FD->doesThisDeclarationHaveABody())
       return FD->doesDeclarationForceExternallyVisibleDefinition();
 
     // Constructors and destructors are required.
     if (FD->hasAttr<ConstructorAttr>() || FD->hasAttr<DestructorAttr>())
       return true;

lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV) {
   const Decl *D = GD.getDecl();
   if (dyn_cast_or_null<NamedDecl>(D))
     setGVProperties(GV, GD);
   else
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
   if (D && D->hasAttr<UsedAttr>())
     addUsedGlobal(GV);
+
+  if (LangOpts.KeepStaticConsts && D && isa<VarDecl>(D)) {
+    const auto *VD = cast<VarDecl>(D);
+    if (VD->getType().isConstQualified() && VD->getStorageClass() == SC_Static)
+      addUsedGlobal(GV);
+  }
 }
 
 bool CodeGenModule::GetCPUAndFeaturesAttributes(const Decl *D,
                                                 llvm::AttrBuilder &Attrs) {
   // Add target-cpu and target-features attributes to functions. If
   // we have a decl for the function and it has a target attribute then
   // parse that and add it to the feature set.
   StringRef TargetCPU = getTarget().getTargetOpts().CPU;

lib/Driver/ToolChains/Clang.cpp

#endif
 
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
   Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
                   options::OPT_fno_emulated_tls);
+  Args.AddLastArg(CmdArgs, options::OPT_fkeep_static_consts);
 
   // AltiVec-like language extensions aren't relevant for assembling.
   if (!isa<PreprocessJobAction>(JA) || Output.getType() != types::TY_PP_Asm)
     Args.AddLastArg(CmdArgs, options::OPT_fzvector);
 
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_show_template_tree);
   Args.AddLastArg(CmdArgs, options::OPT_fno_elide_type);
 

lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   llvm::sort(Opts.ModuleFeatures.begin(), Opts.ModuleFeatures.end());
   Opts.NativeHalfType |= Args.hasArg(OPT_fnative_half_type);
   Opts.NativeHalfArgsAndReturns |= Args.hasArg(OPT_fnative_half_arguments_and_returns);
   // Enable HalfArgsAndReturns if present in Args or if NativeHalfArgsAndReturns
   // is enabled.
   Opts.HalfArgsAndReturns = Args.hasArg(OPT_fallow_half_arguments_and_returns)
                             | Opts.NativeHalfArgsAndReturns;
   Opts.GNUAsm = !Args.hasArg(OPT_fno_gnu_inline_asm);
+  Opts.KeepStaticConsts = Args.hasArg(OPT_fkeep_static_consts);
 
   // __declspec is enabled by default for the PS4 by the driver, and also
   // enabled for Microsoft Extensions or Borland Extensions, here.
   //
   // FIXME: __declspec is also currently enabled for CUDA, but isn't really a
   // CUDA extension. However, it is required for supporting
   // __clang_cuda_builtin_vars.h, which uses __declspec(property). Once that has
   // been rewritten in terms of something more generic, remove the Opts.CUDA

test/CodeGen/keep-static-consts.cpp

+// RUN: %clang_cc1 -fkeep-static-consts -emit-llvm %s -o - -triple=x86_64-unknown-linux-gnu | FileCheck %s
+
+// CHECK: @_ZL7srcvers = internal constant [4 x i8] c"xyz\00", align 1
+// CHECK: @llvm.used = appending global [1 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0)], section "llvm.metadata"
+static const char srcvers[] = "xyz";
+extern const int b = 1;