[PATCH] D104080: [LLD][LLVM] CG Graph profile using relocations

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Are call graph sections expected in fully linked output?



================
Comment at: llvm/lib/MC/MCELFStreamer.cpp:508
+      ".llvm.call-graph-profile", ELF::SHT_LLVM_CALL_GRAPH_PROFILE,
+      ELF::SHF_EXCLUDE, 8 /* sizeof(Elf_CGProfile_Impl<>)*/);
+  PushSection();
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:41-51
+      - Symbol: foo
+        Type: R_X86_64_NONE
+      - Offset: 0x1
+        Symbol: bar
+        Type: R_X86_64_NONE
+      - Offset: 0x2
+        Symbol: bar
----------------
We usually line up the values in a block to make it a little more readable.


================
Comment at: llvm/test/tools/obj2yaml/ELF/call-graph-profile-section.yaml:51
+      - Weight: 98
+  - Name: .rela.llvm.call-graph-profile
+    Type: SHT_RELA
----------------
obj2yaml doesn't care about the relocation section. The code for converting the section to yaml should be independent of the relocations as far as I understand it. I'd remove the relocations to avoid confusing the test. This should allow you then to remove most of the other changes in this test too.


================
Comment at: llvm/test/tools/obj2yaml/ELF/call-graph-profile-section.yaml:90-93
+# INVALID-NEXT:   - Name:            .rela.multiple.8.valid
+# INVALID-NEXT:     Type:            SHT_RELA
+# INVALID-NEXT:     Link:            .symtab
+# INVALID-NEXT:     Info:            .multiple.8.valid
----------------
We're not matching whitespace exactly, so I'd minimise the extra, much like it was before, so that the key and value are closer. This tends to be a little bit more readable. Same goes throughout.


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:48
+# BASIC32:       EntrySize: 12
+# BASIC-NEXT:       SectionData (
+# BASIC64-LE:      0000: 00000000 00000000 00000000 01000000
----------------
Don't check the SectionData of the relocation section. In fact, don't bother having a relocation section at all - this isn't relevant to how yaml2obj generates the call graph section.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1779
+  uint32_t DotCGProfileSecIndex = 0;
+  for (unsigned I = 0, Size = Sections.size(); I < Size; ++I) {
+    const Elf_Shdr &Sec = Sections[I];
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1857-1864
+  // Following LLD lead and doing a seperate loop in case relocation section
+  // comes before section it applies to.
+  for (const Elf_Shdr &Sec : Sections) {
+    if (Sec.sh_info == DotCGProfileSecIndex) {
+      DotCGProfileSecRela = &Sec;
+      break;
+    }
----------------
This code will only allow one .cg_profile section per object, which may or may not be appropriate (it's true that this was the case before, but was it correct then?). Either way, we already have `printRelocatableStackSizes` which finds all `.stack_sizes` sections and their corresponding relocation sections. Maybe rather than having two similar pieces of code that do basically the same thing, but in different ways, we should combine them?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6717
   ListScope L(W, "CGProfile");
-  if (!this->DotCGProfileSec)
+  if (!this->DotCGProfileSec || !this->DotCGProfileSecRela)
     return;
----------------
Seems like it should be some sort of warning and print a partial profile section when the relocations can't be found.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6734-6735
+    this->reportUniqueWarning(
+        "unable to dump the SHT_LLVM_CALL_GRAPH_PROFILE section: " +
+        toString(CGProfileOrErr.takeError()) + "with relocations.");
+    return;
----------------
Error and warning messages shouldn't end with `.` as per the coding standards.

I'd actually rephrase this: `"unable to load relocations for SHT_LLVM_CALL_GRAPH_PROFILE section: " + toString(CGProfileOrErr.takeError())`

This error case needs a test case.

In a similar vein to my earlier comment, we might want to include the section index of the call graph section in this message, so that if there is more than one such section, we can easily identify the problematic one.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6738-6739
+  }
+  assert(CGProfileRelaOrError->size() == (CGProfileOrErr->size() * 2) &&
+         "Number of from/to pairs does not match number of frequencies.");
+
----------------
Seems like this shouldn't be an assert, but a proper warning? If you create a broken object with the wrong number of relocations, this assertion will fire.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6746
+
+  for (uint32_t I = 0, Size = CGProfileOrErr->size(); I < Size; ++I) {
+    const Elf_CGProfile &CGPE = (*CGProfileOrErr)[I];
----------------
Match the return type of `size()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080