[AIX] Turn -fdata-sections on by default in Clang

[jasonliu]

Summary

This patch does the following:

1. Make InitTargetOptionsFromCodeGenFlags() accepts Triple as a parameter, because some options' default value is triple dependant.
2. DataSections is turned on by default on AIX for llc.
3. Clang Driver passes in -fdata-sections by default on AIX.
4. Test cases change accordingly because of the default behaviour change.

[jasonliu]

Not sure if there is a better way to do this. But I have to admit this approach along with D88493 is a bit fragile.
If plug-in writer forgets to set HasExplicitDataSections to true, then the final result that TargetMachine give for getDataSections() could be false on most platform even when they deliberately set DataSections to true.
We have other targets in LLVM that have -fdata-sections by default as well. And they do it differently as well, which makes this a bit more complicate to be consistent.
For example, cloudABI would pass -fdata-sections through the Clang Driver by default. But that approach means if some user just invoke llc directly, they could get the wrong default on llc for that platform.
Wasm would just overwrite the DataSections option to true in their TargetMachine. But that means you could not set it to false even if you want to.
I've thought about making the DataSections option in TargetOptions an enum instead of boolean value, where you could have Default, On and Off to represent what we actually want for the options. That's not a trivial change, as a lot of consumers for TargetOptions are relying it to be a boolean.
If you think it's a better way to solve this problem and I should explore, let me know.

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.def

47

From the current code. I don't see HasExplicitDataSections is necessary. Please remove the lld changes.

clang/lib/Driver/ToolChains/Clang.cpp

4915

You can place AIX in isUseSeparateSections instead.

clang/lib/Frontend/CompilerInvocation.cpp

994

In many cases, there is no need for having both positive and negative CC1 options. The driver handles the default.

[jasonliu]

clang/include/clang/Basic/CodeGenOptions.def

47

The reason I need HasExplicitDataSections goes back to D88493(which I haven't land yet, because I actually need to land these two patches together to avoid build break). In D88493, I'm trying to get the llc's default for -data-sections to be correct for AIX and introduced HasExplicitDataSections. If HasExplicitDataSections is not set, then llc would think there is -data-sections set, so it would just take the target triple's default, which makes DataSections setting to be useless.
It seems there is no good way to set a certain TargetOption's default to be dependant on the current target triple. As I already mentioned in my own comment in this patch, one of the way to achieve that could be make DataSections an enum option in TargetOptions, so that it could have a Default enum which could mean true or false depending on the triple setting. But it could mean a bigger refactoring to this option.

[jasonliu]

clang/include/clang/Basic/CodeGenOptions.def

47

Correction:
If HasExplicitDataSections is not set, then llc would think there is no -data-sections set, so it would just take the target triple's default...

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.def

47

D88748

[jasonliu]

An update follows up with the idea in D88748.

[MaskRay]

lld/Common/TargetOptionsCommandFlags.cpp

17 ↗

(On Diff #295950)

Currently lld does not need Triple. Consider not changing the signature of initTargetOptionsFromCodeGenFlags.

llvm/include/llvm/CodeGen/CommandFlags.h

142 ↗

(On Diff #295950)

If you are going to change the signature, consider renaming it to InitTargetOptionsFromCodeGenFlags to be consistent with the coding standard.

[DiggerLin]

clang/include/clang/Basic/CodeGenOptions.def

46

turn on by default , it should be changed to CODEGENOPT(DataSections , 1, 1) ?

[DiggerLin]

clang/include/clang/Basic/CodeGenOptions.def

46

please ignore above comment. DataSection is set by default only on AIX OS . the comment maybe need to be modified?

[DiggerLin]

clang/test/Driver/aix-data-sections.c

7 ↗

(On Diff #295950)

may be good to check the whether other OS platform behavior be changed or not?

[jasonliu]

clang/test/Driver/aix-data-sections.c

7 ↗

(On Diff #295950)

I think there are existing lit tests checked other platform's behavior.

linux platform behavior is tested in clang/test/Driver/function-sections.c
cloudABI platform behavior is tested in clang/test/Driver/cloudabi.c
msvc behavior is tested in clang/test/Driver/cl-options.c

lld/Common/TargetOptionsCommandFlags.cpp

17 ↗

(On Diff #295950)

This function acts like a forwarder for llvm::codegen::InitTargetOptionsFromCodeGenFlags.
I think it still makes sense to change the signature in this case to minimize the different variation of the function, as those variations cause confusion to people.
I will change the name to match the LLVM style.

[jasonliu]

Change lld's forwarder function's signature to match LLVM style.

[MaskRay]

lld/Common/TargetOptionsCommandFlags.cpp

17 ↗

(On Diff #295950)

If you want to change InitTargetOptionsFromCodeGenFlags in an incompatible way, you can by the way rename it to initTarget*.

lld functions should always stick with the camelCase rule. If there is no meaningful triple, I'd prefer leave out the parameter.

[jasonliu]

lld/Common/TargetOptionsCommandFlags.cpp

17 ↗

(On Diff #295950)

Sorry, didn't realize lld has this camelCase rule.
I don't have a better naming for it, so I will just leave the function signature untouched.

[MaskRay]

A less intrusive approach is to not touch the existing InitTargetOptionsFromCodeGenFlags without parameters. You can add an initTargetOptionsFromCodeGenFlags with const Tripe & as its parameter.

[jasonliu]

In D88737#2321921, @MaskRay wrote:

A less intrusive approach is to not touch the existing InitTargetOptionsFromCodeGenFlags without parameters. You can add an initTargetOptionsFromCodeGenFlags with const Tripe & as its parameter.

I thought about that. But would prefer people think about whether or not a proper Triple is actually needed in that scenario before they make a call to InitTargetOptionsFromCodeGenFlags.
Have an InitTargetOptionsFromCodeGenFlags without parameter, or has Triple() as default parameter could potentially results in people choose the wrong one to call because it's just more convenient.

[DiggerLin]

LGTM

[jasonliu]

Hi Fangrui(@MaskRay), are you okay with this patch to land as is?

[MaskRay]

LGTM.

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(ObjCAutoRefCountExceptions , 1, 0) ///< Whether ARC should be EH-safe.
 CODEGENOPT(Backchain         , 1, 0) ///< -mbackchain
 CODEGENOPT(ControlFlowGuardNoChecks  , 1, 0) ///< -cfguard-no-checks
 CODEGENOPT(ControlFlowGuard  , 1, 0) ///< -cfguard
 CODEGENOPT(CXAAtExit         , 1, 1) ///< Use __cxa_atexit for calling destructors.
 CODEGENOPT(RegisterGlobalDtorsWithAtExit, 1, 1) ///< Use atexit or __cxa_atexit to register global destructors.
 CODEGENOPT(CXXCtorDtorAliases, 1, 0) ///< Emit complete ctors/dtors as linker
                                      ///< aliases to base ctors when possible.
-CODEGENOPT(DataSections      , 1, 0) ///< Set when -fdata-sections is enabled.
+CODEGENOPT(DataSections      , 1, 0) ///< Set by default, or when -f[no-]data-sections.
+CODEGENOPT(HasExplicitDataSections, 1, 0) ///< Set when -f[no-]data-sections is set.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.
 CODEGENOPT(UniqueBasicBlockSectionNames, 1, 1) ///< Set for -funique-basic-block-section-names,
                                                ///< Produce unique section names with
                                                ///< basic block sections.
 ENUM_CODEGENOPT(FramePointer, FramePointerKind, 2, FramePointerKind::None) /// frame-pointer: all,non-leaf,none
 
 CODEGENOPT(DisableFree       , 1, 0) ///< Don't free memory.
 CODEGENOPT(DiscardValueNames , 1, 0) ///< Discard Value Names from the IR (LLVMContext flag)

clang/include/clang/Driver/Options.td

def fwritable_strings : Flag<["-"], "fwritable-strings">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Store string literals as writable data">;
 defm zero_initialized_in_bss : OptOutFFlag<"zero-initialized-in-bss", "", "Don't place zero initialized data in BSS">;
 defm function_sections : OptInFFlag<"function-sections", "Place each function in its own section">;
 def fbasic_block_sections_EQ : Joined<["-"], "fbasic-block-sections=">, Group<f_Group>,
   Flags<[CC1Option, CC1AsOption]>,
   HelpText<"Place each function's basic blocks in unique sections (ELF Only) : all | labels | none | list=<file>">,
   DocBrief<[{Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section.}]>,
   Values<"all,labels,none,list=">;
-defm data_sections : OptInFFlag<"data-sections", "Place each data in its own section">;
+def fdata_sections : Flag<["-"], "fdata-sections">, Group<f_Group>, Flags<[CC1Option]>,
+      HelpText<"Place each data in its own section">;
+def fno_data_sections : Flag<["-"], "fno-data-sections">, Group<f_Group>, Flags<[CC1Option]>;
 defm stack_size_section : OptInFFlag<"stack-size-section", "Emit section containing metadata on function stack sizes">;
 
 defm unique_basic_block_section_names : OptInFFlag<"unique-basic-block-section-names",
  "Use unique names for basic block sections (ELF Only)">;
 defm unique_internal_linkage_names : OptInFFlag<"unique-internal-linkage-names",
  "Uniqueify Internal Linkage Symbol Names by appending the MD5 hash of the module path">;
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;

clang/lib/CodeGen/BackendUtil.cpp

if (!MBOrErr)
           << MBOrErr.getError().message();
     else
       Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
   Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
+  Options.HasExplicitDataSections = CodeGenOpts.HasExplicitDataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
   Options.UniqueBasicBlockSectionNames =
       CodeGenOpts.UniqueBasicBlockSectionNames;
   Options.TLSSize = CodeGenOpts.TLSSize;
   Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
   Options.ExplicitEmulatedTLS = CodeGenOpts.ExplicitEmulatedTLS;
   Options.DebuggerTuning = CodeGenOpts.getDebuggerTuning();
   Options.EmitStackSizeSection = CodeGenOpts.StackSizeSection;

clang/lib/Driver/ToolChains/Clang.cpp

if (Triple.isX86() && Triple.isOSBinFormatELF()) {
       else
         A->render(Args, CmdArgs);
     } else {
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     }
   }
 
-  if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
-                   UseSeparateSections)) {
+  if (Arg *A = Args.getLastArg(options::OPT_fdata_sections,
+                               options::OPT_fno_data_sections)) {
+    if (A->getOption().matches(options::OPT_fdata_sections))
+      CmdArgs.push_back("-fdata-sections");
+    else
+      CmdArgs.push_back("-fno-data-sections");
+  } else if (UseSeparateSections) {
     CmdArgs.push_back("-fdata-sections");
   }
 
   if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
   if (Args.hasFlag(options::OPT_funique_internal_linkage_names,

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.BBSections =
       std::string(Args.getLastArgValue(OPT_fbasic_block_sections_EQ, "none"));
 
   // Basic Block Sections implies Function Sections.
   Opts.FunctionSections =
       Args.hasArg(OPT_ffunction_sections) ||
       (Opts.BBSections != "none" && Opts.BBSections != "labels");
 
+  if (Args.getLastArg(OPT_fdata_sections) ||
+      Args.getLastArg(OPT_fno_data_sections)) {
+    Opts.HasExplicitDataSections = true;
+    Opts.DataSections =
+        Args.hasFlag(OPT_fdata_sections, OPT_fno_data_sections, false);
+  }
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
   Opts.UniqueBasicBlockSectionNames =
       Args.hasArg(OPT_funique_basic_block_section_names);
   Opts.UniqueInternalLinkageNames =
       Args.hasArg(OPT_funique_internal_linkage_names);
 

clang/test/Driver/function-sections.c

 // Test handling of -f(no-)function-sections and -f(no-)data-sections
 //
 // CHECK-FS: -ffunction-sections
 // CHECK-NOFS-NOT: -ffunction-sections
 // CHECK-DS: -fdata-sections
-// CHECK-NODS-NOT: -fdata-sections
+// CHECK-NODS: -fno-data-sections
+// CHECK-DS-DEFAULT-NOT: -fdata-sections
+// CHECK-DS-DEFAULT-NOT: -fno-data-sections
 // CHECK-US-NOT: -fno-unique-section-names
 // CHECK-NOUS: -fno-unique-section-names
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1       \
 // RUN:     -target i386-unknown-linux \
-// RUN:   | FileCheck --check-prefix=CHECK-NOFS --check-prefix=CHECK-NODS %s
+// RUN:   | FileCheck --check-prefix=CHECK-NOFS --check-prefix=CHECK-DS-DEFAULT %s
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1       \
 // RUN:     -target i386-unknown-linux \
 // RUN:     -ffunction-sections \
 // RUN:   | FileCheck --check-prefix=CHECK-FS %s
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1       \
 // RUN:     -target i386-unknown-linux \

lld/COFF/LTO.cpp

 
 static lto::Config createConfig() {
   lto::Config c;
   c.Options = initTargetOptionsFromCodeGenFlags();
 
   // Always emit a section per function/datum with LTO. LLVM LTO should get most
   // of the benefit of linker GC, but there are still opportunities for ICF.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   // Use static reloc model on 32-bit x86 because it usually results in more
   // compact code, and because there are also known code generation bugs when
   // using the PIC model (see PR34306).
   if (config->machine == COFF::IMAGE_FILE_MACHINE_I386)
     c.RelocModel = Reloc::Static;
   else

lld/ELF/LTO.cpp

static lto::Config createConfig() {
 
   // LLD supports the new relocations and address-significance tables.
   c.Options = initTargetOptionsFromCodeGenFlags();
   c.Options.RelaxELFRelocations = true;
   c.Options.EmitAddrsig = true;
 
   // Always emit a section per function/datum with LTO.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   // Check if basic block sections must be used.
   // Allowed values for --lto-basic-block-sections are "all", "labels",
   // "<file name specifying basic block ids>", or none.  This is the equivalent
   // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
     if (config->ltoBasicBlockSections == "all") {

lld/wasm/LTO.cpp

 namespace lld {
 namespace wasm {
 static std::unique_ptr<lto::LTO> createLTO() {
   lto::Config c;
   c.Options = initTargetOptionsFromCodeGenFlags();
 
   // Always emit a section per function/data with LTO.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   c.DisableVerify = config->disableVerify;
   c.DiagHandler = diagnosticHandler;
   c.OptLevel = config->ltoo;
   c.MAttrs = getMAttrs();
   c.CGOptLevel = args::getCGOptLevel(config->ltoo);