This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by quinnp on Jul 8 2022, 3:00 PM.

Details

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.

Diff Detail

Event Timeline

quinnp created this revision.Jul 8 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 3:00 PM
quinnp requested review of this revision.Jul 8 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 3:00 PM
w2yehia added inline comments.Jul 11 2022, 7:27 AM
llvm/lib/LTO/LTOCodeGenerator.cpp
351

any reason we do this for ELF and XCOFF only?

quinnp added inline comments.Jul 11 2022, 7:55 AM
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.

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.

w2yehia accepted this revision.Jul 11 2022, 10:59 AM
This revision is now accepted and ready to land.Jul 11 2022, 10:59 AM
quinnp added a comment.EditedJul 15 2022, 8:50 AM

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 added a comment.EditedJul 18 2022, 8:42 AM

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.

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!

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 updated this revision to Diff 445628.Jul 18 2022, 2:27 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 2:27 PM
MaskRay added inline comments.Jul 18 2022, 2:33 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
581

This should be combined with the previous if

quinnp updated this revision to Diff 445631.Jul 18 2022, 2:35 PM

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 requested changes to this revision.Jul 18 2022, 2:37 PM

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.

This revision now requires changes to proceed.Jul 18 2022, 2:37 PM
quinnp updated this revision to Diff 445634.Jul 18 2022, 2:49 PM

Addressing review comment.

quinnp updated this revision to Diff 445835.Jul 19 2022, 9:02 AM
quinnp marked an inline comment as done.

Fixing test case.

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 updated this revision to Diff 445839.Jul 19 2022, 9:08 AM

Modifying a test to fix check lines.

@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 added a comment.EditedJul 19 2022, 10:28 PM

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
570–573

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 retitled this revision from [libLTO] Set data-sections by default in libLTO for ELF and XCOFF. to [libLTO] Set data-sections by default in libLTO..Jul 20 2022, 7:00 AM
quinnp edited the summary of this revision. (Show Details)
quinnp updated this revision to Diff 446147.Jul 20 2022, 7:01 AM
quinnp marked 6 inline comments as done.

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

quinnp edited the summary of this revision. (Show Details)Jul 20 2022, 7:04 AM

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

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 edited the summary of this revision. (Show Details)Jul 20 2022, 11:55 AM
quinnp updated this revision to Diff 446228.Jul 20 2022, 11:57 AM
quinnp marked an inline comment as done.

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

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

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 added inline comments.Jul 20 2022, 1:17 PM
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 updated this revision to Diff 446597.Jul 21 2022, 12:08 PM
quinnp marked 3 inline comments as done.

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 accepted this revision.Jul 21 2022, 5:10 PM

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

clang/lib/Driver/ToolChains/CommonArgs.cpp
579

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?

This revision is now accepted and ready to land.Jul 21 2022, 5:10 PM
quinnp added inline comments.Jul 25 2022, 7:35 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
579

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 updated this revision to Diff 447336.Jul 25 2022, 7:38 AM
quinnp marked 3 inline comments as done.

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.

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.

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 added inline comments.Jul 26 2022, 5:04 PM
llvm/test/LTO/PowerPC/data-sections-aix.ll
19–20

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

quinnp updated this revision to Diff 448016.Jul 27 2022, 5:52 AM
quinnp marked 5 inline comments as done.

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

This revision was landed with ongoing or failed builds.Jul 27 2022, 6:34 AM
This revision was automatically updated to reflect the committed changes.
quinnp reopened this revision.Jul 27 2022, 7:36 AM
This revision is now accepted and ready to land.Jul 27 2022, 7:36 AM
quinnp updated this revision to Diff 448036.Jul 27 2022, 7:37 AM

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

This revision was landed with ongoing or failed builds.Jul 27 2022, 7:39 AM
This revision was automatically updated to reflect the committed changes.
clang/test/Driver/function-sections.c