[libLTO] Set data-sections by default in libLTO.

[quinnp]

Summary

This patch changes legacy LTO to set data-sections by default. The user can
explicitly unset data-sections. The reason for this patch is to match the
behaviour of lld and gold plugin. Both lld and gold plugin have data-sections on
by default.

This patch also fixes the forwarding of the clang options -fno-data-sections and
-fno-function-sections to libLTO. Now, when -fno-data/function-sections are
specified in clang, -data/function-sections=0 will be passed to libLTO to
explicitly unset data/function-sections.

[w2yehia]

llvm/lib/LTO/LTOCodeGenerator.cpp

351

any reason we do this for ELF and XCOFF only?

[quinnp]

llvm/lib/LTO/LTOCodeGenerator.cpp

351

I don't think there is a particular reason that we do this for ELF and XCOFF only. We needed this fixed for AIX (XCOFF) and wanted to change Linux (ELF) to match the behaviour of lld/gold at the same time. I'm not sure what other file formats need for this so I did not include them.

@hubert.reinterpretcast might have a better answer for this.

[w2yehia]

LGTM

llvm/lib/LTO/LTOCodeGenerator.cpp

351

I don't know either about the other formats, was just wondering.
I think it's safe to do it for the file formats that we know are currently different between libLTO and lld/gold. The proposed change is an improvement with minimal risk.

[quinnp]

I've realized that getExplicitDataSections() interacts with the clang options -fdata-sections and -fno-data-sections differently than the llc versions of these options, --data-sections=true and --data-sections=false. In particular,

• when --data-sections=true is specified with llvm-lto, getExplicitDataSections() returns 1
• when --data-sections=false is specified with llvm-lto, getExplicitDataSections() returns 0
• when no data-sections options are specified with llvm-lto, getExplicitDataSections() returns None.
• when -fdata-sections is specified with clang, getExplicitDataSections() returns 1
• when -fno-data-sections is specified with clang, getExplicitDataSections() returns None.
• when no data-sections options are specified with clang, getExplicitDataSections() returns None.

When getExplicitDataSections() returns None, we may enter the conditional added in this patch because the user did not explicitly set/unset data-sections. However, if getExplicitDataSections() returns 0 or 1, we will not enter the conditional because the user has explicitly set/unset data-sections. Therefore, when running clang -flto -fno-data-sections to link, we may enter the conditional added in this patch despite the user specifying -fno-data-setions because getExplicitDataSections() returned None. The tests added in this patch are testing the llc options with llvm-lto which always prevent the compiler from entering the conditional. I'm not sure if this is a concern because gold plugin is using getExplicitDataSections() in the same way.

[w2yehia]

In D129401#3655316, @quinnp wrote:

I've realized that getExplicitDataSections() interacts with ...

Yes, that seems to be the case that clang forwards the -data-sections option to libLTO/gold only when it's ON. It might be that at that time teh default was assumed to be OFF in libLTO/gold.
Users can still disable it by passing the option manually (i.e. -Wl,-plugin-opt=-data-sections=0).
The forwarding logic in clang shud be fixed at some point.

[quinnp]

In D129401#3660001, @w2yehia wrote:

In D129401#3655316, @quinnp wrote:

I've realized that getExplicitDataSections() interacts with ...

Yes, that seems to be the case that clang forwards the -data-sections option to libLTO/gold only when it's ON. It might be that at that time teh default was assumed to be OFF in libLTO/gold.
Users can still disable it by passing the option manually (i.e. -plugin-opt=-data-sections=0).
The forwarding logic in clang shud be fixed at some point.

Ah I see, that makes sense. Thank you Wael!

[hubert.reinterpretcast]

llvm/lib/LTO/LTOCodeGenerator.cpp

351

I agree with @w2yehia that we should change the data-sections to "on" by default in libLTO for the other file formats where one of lld/the gold plugin sets it to "on".

[quinnp]

Updating patch with a clang change to properly forward -data-sections=0 to libLTO/gold when -fno-data-sections is explicitly specified.

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

580

This should be combined with the previous if

[quinnp]

Updating patch to forward -data-sections=1 to libLTO/gold instead of just -data-sections when -fdata-sections is explicitly specified in clang. This is to be more explicit since -data-sections=0 is now being forwared when -fno-data-sections is explicitly specified.

[MaskRay]

If this is for the legacy LTO interface, please state so. lld/*/LTO.cpp sets c.Options.DataSections = true; to enable data sections by default.

[quinnp]

Addressing review comment.

[quinnp]

Fixing test case.

[quinnp]

If this is for the legacy LTO interface, please state so. lld/*/LTO.cpp sets c.Options.DataSections = true; to enable data sections by default.

Hey @MaskRay, I'm not sure what is considered the legacy LTO interface, but this change is to make the libLTO codegen match the behaviour of LTO used through lld and gold plugin. Both lld and gold plugin turn on data-sections for LTO by default:

• as you mentioned lld/*/LTO.cpp sets c.Options.DataSections = true; by default.
• and llvm/tools/gold/gold-plugin.cpp sets Conf.Options.DataSections = SplitSections; provided that the user did not explicitly set/unset data-sections where SplitSections is true unless gold plugin is doing a relocatable link.

@hubert.reinterpretcast please correct me if I am wrong about why this change is needed.

llvm/lib/LTO/LTOCodeGenerator.cpp

351

@hubert.reinterpretcast I think that if we want to change data-sections to "on" by default for any file format which lld or gold plugin set data-sections to "on", we would set data-sections to "on" for all file formats. This is because gold plugin does not check the file format when it is setting data-sections. You can see where gold plugin sets data-sections here: https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L893

Do you suggest that we remove the checks for file format when setting data-sections in libLTO? ie. change the if statement to this:

if (!codegen::getExplicitDataSections())
  Config.Options.DataSections = true;

[quinnp]

Modifying a test to fix check lines.

[hubert.reinterpretcast]

In D129401#3662857, @quinnp wrote:

@hubert.reinterpretcast please correct me if I am wrong about why this change is needed.

I think your description is correct.

llvm/lib/LTO/LTOCodeGenerator.cpp

351

I can't imagine setting data-sections on by default for LTO being wrong, but I am not sure that the gold-plugin supports the same range of object file formats either. I guess using lld as the precedent is conservatively correct?

[MaskRay]

I made a comment to the llvm/lib/LTO/LTOCodeGenerator.cpp logic.

I think one idea is that only your downstream project sets Config.Options.DataSections = true; , but I can see that having this in the upstream seems fine too to match lld and gold.

clang/lib/Driver/ToolChains/CommonArgs.cpp

572

Shouldn't function-sections be updated as well?

llvm/lib/LTO/LTOCodeGenerator.cpp

352

No need to check ObjectFormat. All (except Mach-O) lld ports default to function-sections/data-sections.

function-sections/data-sections is a no-op in Mach-O and setting the options don't hurt.

[quinnp]

Addressing review comments. Fixing the forwarding for -fno-function-sectons and removing the ObjectFormatType check.

[MaskRay]

Mostly looks good, with a nit in the test and some suggestion to the summary.

In D129401#3662857, @quinnp wrote:

If this is for the legacy LTO interface, please state so. lld/*/LTO.cpp sets c.Options.DataSections = true; to enable data sections by default.

Hey @MaskRay, I'm not sure what is considered the legacy LTO interface, but this change is to make the libLTO codegen match the behaviour of LTO used through lld and gold plugin. Both lld and gold plugin turn on data-sections for LTO by default:

• as you mentioned lld/*/LTO.cpp sets c.Options.DataSections = true; by default.
• and llvm/tools/gold/gold-plugin.cpp sets Conf.Options.DataSections = SplitSections; provided that the user did not explicitly set/unset data-sections where SplitSections is true unless gold plugin is doing a relocatable link.

There is a legacy LTO interface (see llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h) and a resolution-based interface.
Change libLTO in "This patch changes libLTO to set data-sections by default." to legacy LTO.

This patch also fixes the forwarding of the clang options -fno-data-sections and -fno-function-sections to libLTO

This sentence can keep using libLTO or LLVMLTO (the library is LLVMLTO per llvm/lib/LTO/CMakeLists.txt)

llvm/test/LTO/PowerPC/data-sections-linux.ll

10

Using obj2yaml for symbol attributes is overkill and is less readable.

Use llvm-objdump -t

[quinnp]

Addressing review comments. Changing test cases to use llvm-objdump -t instead of obj2yaml.

[quinnp]

In D129401#3666238, @MaskRay wrote:

Mostly looks good, with a nit in the test and some suggestion to the summary.

In D129401#3662857, @quinnp wrote:

If this is for the legacy LTO interface, please state so. lld/*/LTO.cpp sets c.Options.DataSections = true; to enable data sections by default.

Hey @MaskRay, I'm not sure what is considered the legacy LTO interface, but this change is to make the libLTO codegen match the behaviour of LTO used through lld and gold plugin. Both lld and gold plugin turn on data-sections for LTO by default:

• as you mentioned lld/*/LTO.cpp sets c.Options.DataSections = true; by default.
• and llvm/tools/gold/gold-plugin.cpp sets Conf.Options.DataSections = SplitSections; provided that the user did not explicitly set/unset data-sections where SplitSections is true unless gold plugin is doing a relocatable link.

There is a legacy LTO interface (see llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h) and a resolution-based interface.
Change libLTO in "This patch changes libLTO to set data-sections by default." to legacy LTO.

This patch also fixes the forwarding of the clang options -fno-data-sections and -fno-function-sections to libLTO

This sentence can keep using libLTO or LLVMLTO (the library is LLVMLTO per llvm/lib/LTO/CMakeLists.txt)

Ah I see, thank you @MaskRay! I've updated the testcases and the summary.

[MaskRay]

clang/test/Driver/forwarding-sections-liblto.c

2 ↗

(On Diff #446228)

Using a default triple this way does not work on platforms with a default toolchain which doesn't use LTO.

Instead of adding a new file, you may rename gold-lto-sections.c and add more RUN lines there.

Change -target x86_64-unknown-linux to --target=... if you make such a change.

llvm/test/LTO/PowerPC/data-sections-aix.ll

19

Err I want that you test something like 0000000000000000 g F .text {{.*}} f

Is the output not implemented in AIX?

llvm/test/LTO/PowerPC/data-sections-linux.ll

19

Just test one line, something like 0000000000000000 g O .data {{.*}} var

Is the output not implemented in AIX?

[quinnp]

Adressed review comments.

• Modified how llvm-lto test-cases check the llvm-objdump -t output.
• Renamed gold-lto-sections.c to forwarding-sections-liblto.c and modified the test to use the RUN lines from forwarding-sections-liblto.c with the target specified using --target=.

[MaskRay]

There are some nits for tests. I'll step away and just LGTM.

clang/lib/Driver/ToolChains/CommonArgs.cpp

578

Is -plugin-opt=-data-sections=0 a problem for !UseSeparateSections targets?

clang/test/Driver/forwarding-sections-liblto.c

1 ↗

(On Diff #446597)

You may just place these tests into function-sections.c. It seems fine to additionally test data-sections in the file.

(Even if a new test is needed, I'd avoid -liblto and use -lto instead`. But the extra file seems too specific. The test can well be placed in an existing file.)

2 ↗

(On Diff #446597)

Instead of touch %t.o, just use %s

10 ↗

(On Diff #446597)

Just use // CHECK-NOT: "-plugin-opt=-function-sections. No need to duplicate it for 0 and 1.

Ditto for data-sections

llvm/test/LTO/PowerPC/data-sections-linux.ll

21

What does this ...-NOT: .var do?

[quinnp]

clang/lib/Driver/ToolChains/CommonArgs.cpp

578

I don't think -plugin-opt=-data-sections=0 is a problem for !UseSeparateSections targets because we only add "-plugin-opt=-data-sections=0" if the user explicitly specified -fno-data-sections.

• If UseSeparateSections is true, we will enter the if block unless -fno-data-sections is explicitly set.
• If UseSeparateSections is false, we will enter the if block only if -fdata-sections is explicitly set.

Then, if we did not enter the if block, we will only enter the else if block when -fno-data-sections is explicitly set.

llvm/test/LTO/PowerPC/data-sections-linux.ll

21

The line ; CHECK-NO-DATA-SECTIONS-NOT: .var is to ensure that data-sections is correctly turned off. On Linux, when data-sections is on each variable X will have its own section named .bss.X. Here, I am trying to make sure that the the .X part is not present since it may have been consumed by the {{.*}} after the check for .bss on the next line.

[quinnp]

Adressing review comments. Moving tests into function-sections.c, using %s instead of creating a new file with touch, and modifying some CHECK lines to simplify checks.

[hubert.reinterpretcast]

Tests require fixing.

llvm/test/LTO/PowerPC/data-sections-aix.ll

19–20

Unfortunately (without the change) this would pass even if the line was

00000000 g     O .data (csect: .data)   00000000 var

llvm/test/LTO/PowerPC/data-sections-linux.ll

21

This is not needed. The whitespace before the {{.*}} will not match if .bss.var is encountered.

[hubert.reinterpretcast]

With respect to the compiler changes: they look good. I checked, and the injected options being passed to the linker are added before user-specified ones (and that is good).

[MaskRay]

llvm/test/LTO/PowerPC/data-sections-aix.ll

19–20

You can also replace {{.*}} with [[#%x,]] if that improves clarify.

[quinnp]

Addressing review comments: fixing test cases and improving test case clarity.

[quinnp]

Adding lit config to mark tests as unsupported for non PPC targets.

clang/lib/Driver/ToolChains/CommonArgs.cpp

#endif
   }
 
   bool UseSeparateSections =
       isUseSeparateSections(ToolChain.getEffectiveTriple());
 
   if (Args.hasFlag(options::OPT_ffunction_sections,
                    options::OPT_fno_function_sections, UseSeparateSections)) {
     CmdArgs.push_back("-plugin-opt=-function-sections");
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
-                   UseSeparateSections)) {
-    CmdArgs.push_back("-plugin-opt=-data-sections");
-  }
+                   UseSeparateSections))
+    CmdArgs.push_back("-plugin-opt=-data-sections=1");
+  else if (Args.hasArg(options::OPT_fno_data_sections))
+    CmdArgs.push_back("-plugin-opt=-data-sections=0");
 
   // Pass an option to enable split machine functions.
   if (auto *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
                                 options::OPT_fno_split_machine_functions)) {
     if (A->getOption().matches(options::OPT_fsplit_machine_functions))
       CmdArgs.push_back("-plugin-opt=-split-machine-functions");
   }
 
   if (Arg *A = getLastProfileSampleUseArg(Args)) {
     StringRef FName = A->getValue();

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

+// RUN: touch %t.o
+// RUN: %clang %t.o -### -flto 2>&1 | FileCheck %s
+// RUN: %clang %t.o -### -flto 2>&1 -fdata-sections | FileCheck %s \
+// RUN:   --check-prefix=CHECK-DATA-SECTIONS
+// RUN: %clang %t.o -### -flto 2>&1 -fno-data-sections | FileCheck %s \
+// RUN:   --check-prefix=CHECK-NO-DATA-SECTIONS
+
+// CHECK-NOT: "-plugin-opt=-data-sections"
+// CHECK-DATA-SECTIONS: "-plugin-opt=-data-sections=1"
+// CHECK-NO-DATA-SECTIONS: "-plugin-opt=-data-sections=0"

llvm/lib/LTO/LTOCodeGenerator.cpp

 #include "llvm/LTO/legacy/LTOCodeGenerator.h"
 
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/Passes.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/CodeGen/CommandFlags.h"
 #include "llvm/CodeGen/ParallelCG.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/Config/config.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
else if (Triple.getArch() == llvm::Triple::x86)
       Config.CPU = "yonah";
     else if (Triple.isArm64e())
       Config.CPU = "apple-a12";
     else if (Triple.getArch() == llvm::Triple::aarch64 ||
              Triple.getArch() == llvm::Triple::aarch64_32)
       Config.CPU = "cyclone";
   }
 
+  // If data-sections is not explicitly set or unset, set data-sections by
+  // default for ELF and XCOFF to match the behaviour of lld and gold plugin.
+  llvm::Triple::ObjectFormatType ObjectFormat = Triple.getObjectFormat();
+  if (!codegen::getExplicitDataSections() &&
+      (ObjectFormat == llvm::Triple::ObjectFormatType::ELF ||
+       ObjectFormat == llvm::Triple::ObjectFormatType::XCOFF))
+    Config.Options.DataSections = true;
+
   TargetMach = createTargetMachine();
   assert(TargetMach && "Unable to create target machine");
 
   return true;
 }
 
 std::unique_ptr<TargetMachine> LTOCodeGenerator::createTargetMachine() {
   assert(MArch && "MArch is not set!");

llvm/test/LTO/PowerPC/data-sections-aix.ll

+; RUN: rm -rf %t
+; RUN: mkdir %t
+; RUN: llvm-as %s -o %t/bc.bc
+; RUN: llvm-lto -exported-symbol var -O0 %t/bc.bc -o %t/default.o
+; RUN: llvm-lto -exported-symbol var -O0 --data-sections=true %t/bc.bc -o \
+; RUN:   %t/data-sections.o
+; RUN: llvm-lto -exported-symbol var -O0 --data-sections=false %t/bc.bc -o \
+; RUN:   %t/no-data-sections.o
+; RUN: obj2yaml %t/default.o | FileCheck --match-full-lines %s
+; RUN: obj2yaml %t/data-sections.o | FileCheck --match-full-lines %s
+; RUN: obj2yaml %t/no-data-sections.o | FileCheck --match-full-lines \
+; RUN:   --check-prefix CHECK-NO-DATA-SECTIONS %s
+
+target triple = "powerpc-ibm-aix7.2.0.0"
+
+@var = global i32 0
+
+; CHECK:                  Symbols:
+; CHECK-NOT:                - Name: .data
+
+; CHECK-NO-DATA-SECTIONS: Symbols:
+; CHECK-NO-DATA-SECTIONS:   - Name: .data

llvm/test/LTO/PowerPC/data-sections-linux.ll

+; RUN: rm -rf %t
+; RUN: mkdir %t
+; RUN: llvm-as %s -o %t/bc.bc
+; RUN: llvm-lto -exported-symbol var -O0 %t/bc.bc -o %t/default.o
+; RUN: llvm-lto -exported-symbol var -O0 --data-sections=true %t/bc.bc -o \
+; RUN:   %t/data-sections.o
+; RUN: llvm-lto -exported-symbol var -O0 --data-sections=false %t/bc.bc -o \
+; RUN:   %t/no-data-sections.o
+; RUN: obj2yaml %t/default.o | FileCheck --match-full-lines %s
+; RUN: obj2yaml %t/data-sections.o | FileCheck --match-full-lines %s
+; RUN: obj2yaml %t/no-data-sections.o | FileCheck --match-full-lines \
+; RUN:   --check-prefix CHECK-NO-DATA-SECTIONS %s
+
+target triple = "powerpc64le-unknown-linux-gnu"
+
+@var = global i32 0
+
+; CHECK:                       Symbols:
+; CHECK:                         - Name:    var
+; CHECK-NEXT:                      Type:    STT_OBJECT
+; CHECK-NEXT:                      Section: .bss.var
+
+; CHECK-NO-DATA-SECTIONS:      Symbols:
+; CHECK-NO-DATA-SECTIONS:        - Name:    var
+; CHECK-NO-DATA-SECTIONS-NEXT:     Type:    STT_OBJECT
+; CHECK-NO-DATA-SECTIONS-NEXT:     Section: .bss