[PATCH] D88717: [obj2yaml] [yaml2obj] Add yaml support for SHT_LLVM_BB_ADDR_MAP section.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Generally looks fine to me, I've put a few comments inline and also:

1. This needs a rebasing (D89039 <https://reviews.llvm.org/D89039> was landed, it added support of "Size"/"Content" for all sections).
2. This needs `yaml2obj` tests. I think following are reasonable for initial support: a) Check values of section fields produced by default (sh_link, sh_info, etc). b) Check you can specify none of "Content", "Size" or "Entries" to produce an empty section. c) Check you can specify only "Size", only "Content" or only "Entries" (see tests in D89039 <https://reviews.llvm.org/D89039> and others). d) Check you can't specify "Entries" with "Content"/"Size" (you need to add the code to `validate()`, see D89391 <https://reviews.llvm.org/D89391>).



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1311
+    SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(E.NumBlocks);
+    if (!E.BBEntries.hasValue())
+      continue;
----------------



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1317
+                         CBA.writeULEB128(BBE.Metadata);
+    }
+  }
----------------
You don't need to have curly braces here.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1670
+  assert(IO.getContext() && "The IO context is not initialized");
+  IO.mapOptional("Address", E.Address, Hex64(0));
+  IO.mapRequired("NumBlocks", E.NumBlocks);
----------------
This needs a yaml2obj test to show that we can omit "Address" and it will accept it and produce "0" as address.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1672
+  IO.mapRequired("NumBlocks", E.NumBlocks);
+  IO.mapOptional("BBEntries", E.BBEntries);
+}
----------------
I think you don't need `NumBlock` YAML key. It matches the number of items in "BBEntries", right?
E.g. see how we describe `Notes` (and other things):

```
  - Name:  .note.foo
    Type:  SHT_NOTE
    Flags: [ SHF_ALLOC ]
    Notes:
      - Name: AB
        Desc: ''
        Type: 0xFF
      - Name: ABC
        Desc: '123456'
        Type: 254
```


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:147
+# MULTI-NEXT:         BBEntries: []
+--- !ELF
+FileHeader:
----------------
Missing empty line before "--- !ELF"


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:156
+    Entries:
+      - Address:   0x0000000000000010
+        NumBlocks: 0x00000001
----------------
I'd change `0x0000000000000010` to `0x0000000000000000` and demonstrate that
obj2yaml does not emit the "Address" key when it is 0x0. (I haven't check, but this is how I expect the code works now).


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:511
           [this](const Elf_Shdr *S) { return dumpCallGraphProfileSection(S); };
+
+    case ELF::SHT_LLVM_BB_ADDR_MAP:
----------------
Excessive empty line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88717