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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Test case(s) for the LLD portion of this patch?



================
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>
----------------
A quick skim of the area shows `//` is used for comenting function-level comments.


================
Comment at: lld/ELF/Driver.cpp:886
+        break;
+      } else if (sec.sh_type == SHT_REL) {
+        ArrayRef<typename ELFT::Rel> rels =
----------------



================
Comment at: lld/ELF/InputFiles.h:24
 #include "llvm/Support/Threading.h"
+#include <cstdint>
 #include <map>
----------------
Do you need to add this include? My understanding of the style guide is that you don't need to add it, unless the code doesn't compile without (it is likely pulled in by other headers).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6705-6706
 
+/// Returns true if rel/rela section exists, and populates SymbolIndices.
+/// Otherwise returns false.
+template <class ELFT>
----------------
Style is overwhelmingly `//` for functions here. These don't need doxygen comments, since they're not part of a public interface.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6733
+  } else {
+    // MC unconditionally produces SHT_REL, but GNU strip/objcopy may convert the format
+    // to SHT_RELA (https://sourceware.org/bugzilla/show_bug.cgi?id=28035)
----------------
clang-format


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6739-6741
+      Dumper->reportUniqueWarning("unable to load relocations for "
+                                  "SHT_LLVM_CALL_GRAPH_PROFILE section: " +
+                                  toString(CGProfileRelaOrError.takeError()));
----------------
Test case needed for this path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105217