MaskRay added a comment. Mostly looks good, with a nit in the test and some suggestion to the summary.
In D129401#3662857 <https://reviews.llvm.org/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) ================ Comment at: llvm/test/LTO/PowerPC/data-sections-linux.ll:9 +; 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 ---------------- Using obj2yaml for symbol attributes is overkill and is less readable. Use `llvm-objdump -t` 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
