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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
