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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked an inline comment as done.
rahmanl added a comment.

Thanks for your helpful comments @grimar.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1311
+    SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(E.NumBlocks);
+    if (!E.BBEntries.hasValue())
+      continue;
----------------
grimar wrote:
> 
Adopted in the line above.


================
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);
----------------
grimar wrote:
> This needs a yaml2obj test to show that we can omit "Address" and it will accept it and produce "0" as address.
Please look at the added test.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1672
+  IO.mapRequired("NumBlocks", E.NumBlocks);
+  IO.mapOptional("BBEntries", E.BBEntries);
+}
----------------
jhenderson wrote:
> grimar wrote:
> > 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
> > ```
> Presumably if the NumBlock field corresponds to part of the data structure, we want it to be an Optional member, so that users can override it independently of the number of entries - that way tests can be crafted that show what happens if the value is invalid.
Great suggestion. Thanks. Removed the NumBlocks YAML key, even though the field still shows up in the section data.


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:156
+    Entries:
+      - Address:   0x0000000000000010
+        NumBlocks: 0x00000001
----------------
grimar wrote:
> 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).
Thanks. It works as expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88717