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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

You also need a yaml2obj test case. This is adding new functionality. Any such patch needs a test case.

> Oh. I see. Something like this? I guess downside is that linking time will go up slightly because we have to scan through all the sections again.

Yes, this is close. See my inline comment. The downside is likely negligible if you benchmark it... sizeof(InputSection) is ~184 bytes. I would be unhappy if the relatively minor feature takes 15% of its size.



================
Comment at: lld/ELF/Driver.cpp:864
+    symbolIndices.push_back(rel.getSymbol(config->isMips64EL));
+  }
+}
----------------
MaskRay wrote:
> drop braces
Not addressed, but see below: with inlining you can ignore this.


================
Comment at: lld/ELF/Driver.cpp:876
+
+  // SHT_LLVM_CALL_GRAPH_PROFILE Section Index.
+  size_t cgProfileSectionIndex = 0;
----------------
section index


================
Comment at: lld/ELF/Driver.cpp:886
+
+  for (size_t i = 0, e = objSections.size(); i < e; ++i) {
+    const Elf_Shdr_Impl<ELFT> &sec = objSections[i];
----------------
`<` => `!=`


================
Comment at: lld/ELF/Driver.cpp:891
+        if (sec.sh_type == SHT_RELA)
+          getIndices(symbolIndices,
+                     CHECK(obj.relas(sec),
----------------
just inline `getIndices` here


================
Comment at: lld/ELF/InputFiles.cpp:580
 
-    if (sec.sh_type == ELF::SHT_LLVM_CALL_GRAPH_PROFILE) {
+    if (sec.sh_type == ELF::SHT_LLVM_CALL_GRAPH_PROFILE)
       cgProfile =
----------------
Replace `cgProfile` with `cgProfileSectionIndex`. Let `processRelocationsCGSection` (which probably should be renamed to something else, e.g. `processCallGraphRelocations`) bail out if `cgProfileSectionIndex` is 0 (SHN_UNDEF). You can also delete one loop there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105217