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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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();
----------------
jhenderson wrote:
> 
This hasn't been addressed properly: the `=` at the end of the comment in my inline edit is important, as clang-format interprets such a pattern differently (note that if you add the equals sign and then reformat, the space between the comment and 8 will disappear.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:130
+
+## Check we report a warning when relocation section is not present.
+# RUN: yaml2obj %s --docnum=3 -o %t4.o
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:161
+
+## Check we report a warning when relocation section entries do not match call graph entries.
+# RUN: yaml2obj %s --docnum=4 -o %t5.o
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test:211
+
+## Check we report a warning when relocation section cant't be loaded.
+# RUN: yaml2obj %s --docnum=5 -o %t6.o
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:84
 
 ## Check we can refer to symbols by name.
 # RUN: yaml2obj --docnum=3 %s -o %t.sym
----------------
I think you can drop this entire test case, since the YAML no longer refers to symbols for the call graph section.


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:134
 
-## Check we can't reference unknown symbols by name.
-# RUN: not yaml2obj --docnum=5 %s 2>&1 | FileCheck %s --check-prefix=UNKNOWN-NAME
-# RUN: not yaml2obj --docnum=6 %s 2>&1 | FileCheck %s --check-prefix=UNKNOWN-NAME
-# UNKNOWN-NAME: error: unknown symbol referenced: 'bar' by YAML section '.llvm.call-graph-profile'
-
---- !ELF
-FileHeader:
-  Class: ELFCLASS64
-  Data:  ELFDATA2LSB
-  Type:  ET_DYN
-Sections:
-  - Name: .llvm.call-graph-profile
-    Type: SHT_LLVM_CALL_GRAPH_PROFILE
-## The first symbol is valid, but the second is unknown.
-    Entries:
-      - From:   foo
-        To:     bar
-        Weight: 10
-Symbols:
-  - Name: foo
-
---- !ELF
-FileHeader:
-  Class: ELFCLASS64
-  Data:  ELFDATA2LSB
-  Type:  ET_DYN
-Sections:
-  - Name: .llvm.call-graph-profile
-    Type: SHT_LLVM_CALL_GRAPH_PROFILE
-## The first symbol is unknown, but the second is valid.
-    Entries:
-      - From:   bar
-        To:     foo
-        Weight: 10
-Symbols:
-  - Name: foo
-
 ## Check we can specify arbitrary symbol indexes for an SHT_LLVM_CALL_GRAPH_PROFILE section entry.
+# RUN: yaml2obj --docnum=5 %s -o %t.unk
----------------
Ditto: we no longer refer to symbols directly in the YAML for this section, so drop this case.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6732-6733
+                                  "SHT_LLVM_CALL_GRAPH_PROFILE section: " +
+                                  toString(CGProfileRelaOrError.takeError()) +
+                                  "with relocations");
+        UseReloc = false;
----------------
Get rid of the unnecessary "with relocations".


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6740
+        this->reportUniqueWarning(
+            "Number of from/to pairs does not match number of frequencies");
+        UseReloc = false;
----------------
The style is no capital for the first letter of warnings.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6745
+      this->reportUniqueWarning(
+          "Relocation section for a call graph section doesn't exist");
+
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6747
+
+    auto GetIndex = [&](uint32_t Index) -> uint32_t {
+      const Elf_Rel_Impl<ELFT, true> &Rel = CGProfileRela[Index];
----------------
The return type of `Rel.getSymbol(...)` is already `uint32_t`, so no need for the trailing return type to be explicitly specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080