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 ↗

(On Diff #161307)

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 ↗

(On Diff #161307)

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/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">;
 def fcxx_static_destructors : Flag<["-"], "fc++-static-destructors">,
   Group<f_Group>,
   HelpText<"Enable C++ static destructor registration (the default)">;
 def fno_cxx_static_destructors : Flag<["-"], "fno-c++-static-destructors">,

include/clang/Frontend/CodeGenOptions.def

 /// Whether to emit all vtables
 CODEGENOPT(ForceEmitVTables, 1, 0)
 
 /// Whether to emit an address-significance table into the object file.
 CODEGENOPT(Addrsig, 1, 0)
 
 ENUM_CODEGENOPT(SignReturnAddress, SignReturnAddressScope, 2, None)
 
+/// Whether to emit unused static constants.
+CODEGENOPT(KeepStaticConsts, 1, 0)
+
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
 

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 (CodeGenOpts.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;
bool CodeGenModule::imbueXRayAttrs(llvm::Function *Fn, SourceLocation Loc,
   return true;
 }
 
 bool CodeGenModule::MustBeEmitted(const ValueDecl *Global) {
   // Never defer when EmitAllDecls is specified.
   if (LangOpts.EmitAllDecls)
     return true;
 
+  if (CodeGenOpts.KeepStaticConsts) {
+    const auto *VD = dyn_cast<VarDecl>(Global);
+    if (VD && VD->getType().isConstQualified() &&
+        VD->getStorageClass() == SC_Static)
+      return true;
+  }
+
   return getContext().DeclMustBeEmitted(Global);
 }
 
 bool CodeGenModule::MayBeEmittedEagerly(const ValueDecl *Global) {
   if (const auto *FD = dyn_cast<FunctionDecl>(Global))
     if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
       // Implicit template instantiations may change linkage if they are later
       // explicitly instantiated, so they should not be emitted eagerly.

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

if (Arg *A = Args.getLastArg(OPT_msign_return_address)) {
     else if (SignScope.equals_lower("non-leaf"))
       Opts.setSignReturnAddress(
           CodeGenOptions::SignReturnAddressScope::NonLeaf);
     else
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
   }
 
+  Opts.KeepStaticConsts = Args.hasArg(OPT_fkeep_static_consts);
+
   return Success;
 }
 
 static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
                                       ArgList &Args) {
   Opts.OutputFile = Args.getLastArgValue(OPT_dependency_file);
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);

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;