[PATCH] D105217: [LLD] Adding support for RELA for CG Profile.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lld/ELF/Driver.cpp:858-859
 
-template <class ELFT> static void readCallGraphsFromObjectFiles() {
-  auto getIndex = [&](ObjFile<ELFT> *obj, uint32_t index) {
-    const Elf_Rel_Impl<ELFT, false> &rel = obj->cgProfileRel[index];
-    return rel.getSymbol(config->isMips64EL);
-  };
+// If SHT_LLVM_CALL_GRAPH_PROFILE and it's relocation section exists returns
+// true, and populates cgProfile and symbolIndices.
+template <class ELFT>
----------------



================
Comment at: lld/ELF/Driver.cpp:886
+        break;
+      } else if (sec.sh_type == SHT_REL) {
+        ArrayRef<typename ELFT::Rel> rels =
----------------
jhenderson wrote:
> 
Ping? This hasn't been addressed...


================
Comment at: lld/test/ELF/cgprofile-rela-obj.test:1
+# REQUIRES: x86
+
----------------
Add a comment (using '##' for comment markers) to this test to explain the purpose of this test.

I think you can also drop `-obj` from the test name.


================
Comment at: lld/test/ELF/cgprofile-rela-obj.test:4
+# RUN: yaml2obj %s -o %t.o
+# RUN: ld.lld -e A %t.o -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
You don't need to specify the entry point explicitly. If you really want to suppress the warning, I'd just rename one of the symbols to `_start`.


================
Comment at: lld/test/ELF/cgprofile-rela-obj.test:8
+# RUN: llvm-nm --no-sort %t | FileCheck %s --check-prefix=NO-CG
+--- !ELF
+FileHeader:
----------------
Nit: add a blank line between the RUN and YAML blocks.

I also have a personal preference for the flow of the test to be:
```
# RUN commands

# CHECK directives

YAML
```


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:209
 
-## Check we report a warning when a relocation section cant't be loaded.
+## Check we report a warning when a relocation, REL, section cant't be loaded.
 # RUN: yaml2obj %s --docnum=5 -o %t6.o
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:307
+
+## Check we report a warning when a relocation, RELA, section cant't be loaded.
+# RUN: yaml2obj %s --docnum=7 -o %t8.o
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105217