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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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
----------------
ayermolo wrote:
> jhenderson wrote:
> > 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.
> I think relocation is needed in this case. Check is done with llvm-readobj --cg-profile. Unless that flag can be dropped?
The llvm-readobj check of the cg-profile data is a nice-to-have to confirm that our tools are self-consistent. However, I'm not sure it's really necessary, since we have a dedicated llvm-readobj test that uses the yaml output, which covers this sort of case.


================
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;
+    }
----------------
ayermolo wrote:
> jhenderson wrote:
> > 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?
> Hmm, well from my understanding if it comes from clang side it should be one per object file. I guess someone can construct object with multiple ones, using say yaml2obj, but what would be practical point of it? LLD right now doesn't support that scenario.
> I looked in to printRelocatableStackSizes, not sure there is enough code worth combining, but seems like a good way to keep all the code related to printing cg profile in one place. Let me refactor and see what comes out of it.
I was thinking the factored out code would be something like:

```
std::vector<std::pair<Elf_Shdr*, Elf_Shdr*>> getSectionAndRelocations(std::function<bool(Elf_Shdr*)> IsMatch) {/*...*/}
```

where each of the different users would provide their own predicate (i.e. name or type check in the cg profile and stack sizes cases). Types in the example are just a rough idea. In the cgprofile case, we can just pick one pair from the vector.


================
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.");
+
----------------
ayermolo wrote:
> jhenderson wrote:
> > 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.
> I can change it to warning, but then will need to add a check not to execute a loop.
This is what I had in mind, and is similar to the pattern elsewhere:
```
if (CGProfileRelaOrError->size() != (CGProfileOrErr->size() * 2)) {
  reportUniqueWarning(...);
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080