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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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
----------------
grimar wrote:
> 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?
> 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?

That sounds fair. Thanks for the explanation!


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

https://reviews.llvm.org/D73788