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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:901
+template <class ELFT> static void readCallGraphsFromObjectFiles() {
+  SmallVector<uint32_t, 128> symbolIndices;
+  ArrayRef<typename ELFT::CGProfile> cgProfile;
----------------
This consumes too much stack space. Perhaps `SmallVector<uint32_t, 32>`

lld already uses `SmallVector<unsigned, 32>` so using 32 will not increase binary size.


================
Comment at: lld/ELF/Driver.cpp:905
     auto *obj = cast<ObjFile<ELFT>>(file);
-    if (obj->cgProfileRel.empty())
+
+    if (!processCallGraphRelocations(symbolIndices, cgProfile, obj))
----------------
Delete the unneeded blank line


================
Comment at: lld/ELF/InputFiles.h:253
+  // SHT_LLVM_CALL_GRAPH_PROFILE section index.
+  size_t cgProfileSectionIndex = 0;
 
----------------
uint32_t


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:259
+
+# RUN: yaml2obj %s --docnum=6 -o %t7.o
+# RUN: llvm-readobj %t7.o --cg-profile | FileCheck %s --check-prefix=LLVM-RELA
----------------
Add a test comment: `## GNU strip may convert SHT_REL to SHT_RELA. Test we can handle SHT_RELA.`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:262
+# RUN: llvm-readelf %t7.o --cg-profile | FileCheck %s --check-prefix=GNU-RELA
+# RUN: llvm-readobj %t7.o --elf-cg-profile | FileCheck %s --check-prefix=LLVM-RELA
+# RUN: llvm-readelf %t7.o --elf-cg-profile | FileCheck %s --check-prefix=GNU-RELA
----------------
Delete elf-cg-profile RUN lines. The aliases are tested by dedicated tests, no need for duplicating.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:292
+      - Weight: 98
+    EntSize: [[ENTSIZE=<none>]]
+  - Name: .rela.llvm.call-graph-profile
----------------
Delete if unused or make it match the reality


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:299
+        Type:   R_X86_64_NONE
+      - Offset: 0x1
+        Symbol: bar
----------------
The offsets should match the reality: they relocate the values (0x0, 0x8, 0x10, ...)


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6727
+      return false;
+    } else
+      CGProfileRel = *CGProfileRelOrError;
----------------
Drop `else` since we are using the early return pattern.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6733
+  } else {
+    typename ELFT::RelaRange CGProfileRela;
+    Expected<typename ELFT::RelaRange> CGProfileRelaOrError =
----------------
Add a comment: MC unconditionally produces SHT_REL but GNU strip may convert the format to SHT_RELA (https://sourceware.org/bugzilla/show_bug.cgi?id=28035)

I don't expect they will fix this any time soon, but good to have a reference to justify additional complexity here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105217