[PATCH] D129401: [libLTO] Set data-sections by default in libLTO for ELF and XCOFF.

[Quinn Pham via Phabricator via llvm-commits]

quinnp added a comment.

> 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.



================
Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:351
+  llvm::Triple::ObjectFormatType ObjectFormat = Triple.getObjectFormat();
+  if (!codegen::getExplicitDataSections() &&
+      (ObjectFormat == llvm::Triple::ObjectFormatType::ELF ||
----------------
hubert.reinterpretcast wrote:
> w2yehia wrote:
> > quinnp wrote:
> > > w2yehia wrote:
> > > > any reason we do this for ELF and XCOFF only?
> > > 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.
> > 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.
> 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".
@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;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129401/new/

https://reviews.llvm.org/D129401