[PATCH] D73788: [yaml2obj/obj2yaml] - Add support for the SHT_LLVM_CALL_GRAPH_PROFILE sections.

[George Rimar via Phabricator via llvm-commits]

grimar marked 4 inline comments as done.
grimar added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/call-graph-profile-section.yaml:7
+# RUN: obj2yaml %t.le64 | FileCheck %s --check-prefix=BASIC
+# RUN: yaml2obj --docnum=2 %s -o %t.be64
+# RUN: obj2yaml %t.be64 | FileCheck %s --check-prefix=BASIC
----------------
MaskRay wrote:
> This is verbose. Intention is to test that the section has the same representation with different EI_CLASS.
> 
> If https://reviews.llvm.org/D73821 looks good, we can add `-D EI_CLASS=ELFCLASS32` and `-D EI_DATA=ELFDATA2LSB` to represent a similar object file.
> This is verbose.

Yes, that is why I placed the "TODO" comment below.


================
Comment at: llvm/test/tools/obj2yaml/call-graph-profile-section.yaml:202-203
+    Content: "00112233445566778899AABBCCDDEEFF"
+## Case 3: Check we use the "Content" property when unable to match a
+##         symbol index to a symbol.
+  - Name: .unknown.symbol.1
----------------
jhenderson wrote:
> Why do we do this? Can't we write the invalid index as  it is in the YAML below?
Technially it is possible, but there is a problem behind:
When we read a binary (in obj2yaml) we read the `From`/`To` fields as an integers.
If we can't map them to a symbol and want to store them as `StringRef`, we need to convert them
to strings and save those strings somewhere
(the case with yaml2obj is different as it has strings representation from the begining).

The solution I used for `AddrsigSymbol` was:

```
struct AddrsigSymbol {
  AddrsigSymbol(StringRef N) : Name(N), Index(None) {}
  AddrsigSymbol(llvm::yaml::Hex32 Ndx) : Name(None), Index(Ndx) {}
  AddrsigSymbol() : Name(None), Index(None) {}

  Optional<StringRef> Name;
  Optional<llvm::yaml::Hex32> Index;
};
```

i.e. we can convert `AddrsigSymbol` to `SymbolOrName` and reuse probably.
But I'd like to investigate another options first, perhaps it is possible to eliminate `AddrsigSymbol`.

Given all above I supposed that it should be done separatelly as it should
change/refactor another code and hence probably deserves a separate follow-up patch.
What do you think?


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

https://reviews.llvm.org/D73788