[PATCH] D146778: [lld] Preliminary fat-lto-object support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

Thanks. This looks mostly good but archives need to be implemented.

> If the -fat-lto-objects flag is passed to LLD and the input files are fat

If the `--fat-lto-objects` option is ...



================
Comment at: lld/ELF/Driver.cpp:315
+          IRObjectFile::findBitcodeInMemBuffer(mbref);
+      if (!errorToBool(fatLTOData.takeError())) {
+        files.push_back(make<BitcodeFile>(*fatLTOData, "", 0, inLib));
----------------
Add a test that the `.llvm.lto` has invalid LLVM IR.

You can use `.section .llvm.lto,"e"` in assembly to construct such a test case.


================
Comment at: lld/ELF/Driver.cpp:2808
+
+        // Do not aggregate the .llvm.lto section
+        if (s->name == ".llvm.lto")
----------------
This code can be avoided if `.llvm.lto` has the SHF_EXCLUDE flag.


================
Comment at: lld/ELF/Options.td:633
 
+defm fatlto_objects: B<"fat-lto-objects",
+    "Use the embedded bitcode in the .llvm.lto section of the object file",
----------------
`BB`

Don't allow single-dash long options for new options.

`defm fat_lto_objects`


================
Comment at: lld/docs/ReleaseNotes.rst:34
 
+- Support LTO with LLVM FatLTO objects by adding the `-fat-lto-objects` flag.
+  Without `-fat-lto-objects`, LLD will link Fat LTO objects without LTO.
----------------
Double backsticks. Prefer `option` over `flag` especially for linkers.


================
Comment at: lld/test/ELF/fatlto/Inputs/a-LTO.ll:1
+; ModuleID = 'a-LTO.bc'
+source_filename = "a.c"
----------------
Extra files can be avoided with `split-file`


================
Comment at: lld/test/ELF/fatlto/Inputs/a-LTO.ll:5
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
----------------
Add a `_start` function to prevent a linker warning. Maybe just change a to `_start`


================
Comment at: lld/test/ELF/fatlto/fatlto.test:4
+
+;; Clean up and initialize test dir
+; RUN: rm -rf %t
----------------
Add period for complete sentences throughout.

Delete `;; Clean up and initialize test dir` which is not too useful.


================
Comment at: lld/test/ELF/fatlto/fatlto.test:5
+;; Clean up and initialize test dir
+; RUN: rm -rf %t
+; RUN: mkdir -p %t
----------------
`; RUN: rm -rf %t && mkdir %t && cd %t` 

Do this in one line and you can avoid `%t/` below...


================
Comment at: lld/test/ELF/fatlto/fatlto.test:24
+; Final executable should not have .llvm.lto section no matter what the target is
+; RUN: ld.lld -o %t/foo-fatLTO %t/a-fatLTO.o %t/main-fatLTO.o -fat-lto-objects
+; RUN: llvm-readobj -S %t/foo-fatLTO | FileCheck --check-prefix=CHECK-LTO-TARGET %s
----------------
`--fat-lto-objects`


================
Comment at: lld/test/ELF/fatlto/fatlto.test:29
+; CHECK-LTO-TARGET-NOT: Name: .llvm.lto
+
+;; Final executable should not have .llvm.lto section no matter what the target is
----------------
We should test `--start-lib` (`inLib`) and fat lto objects in an archive.


================
Comment at: lld/test/ELF/fatlto/fatlto.test:53
+; RUN: ld.lld -o %t/foo-noLTO %t/a.o %t/main.o
+; RUN: obj2yaml %t/foo-fatNoLTO > %t/foo-fatNoLTO.yaml
+; RUN: obj2yaml %t/foo-noLTO > %t/foo-noLTO.yaml
----------------
This obj2yaml comparison is not common. Just use `cmp foo-fatNoLTO foo-noLTO`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146778