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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D129401#3662857 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:351
+  llvm::Triple::ObjectFormatType ObjectFormat = Triple.getObjectFormat();
+  if (!codegen::getExplicitDataSections() &&
+      (ObjectFormat == llvm::Triple::ObjectFormatType::ELF ||
----------------
quinnp wrote:
> 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;
> ```
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129401