[PATCH] D95511: [llvm-readelf] Support dumping the BB address map section with --bb-addr-map.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You need to add this option to the llvm-readobj and llvm-readelf command guides under llvm/docs/CommandGuide.

Ideally, I think you should add gtest unit tests for the libObject code. The lit test would then just (primarily) test the llvm-readobj handling of the potential return values of the parsing code.



================
Comment at: llvm/include/llvm/Object/ELF.h:281
+  Expected<std::vector<Elf_BBAddrMap>>
+  decode_bbaddrmap(const Elf_Shdr &Sec) const;
+
----------------
I'm not sure why some functions use `_` instead of camelCase, but it's probably best you use the latter. Maybe put it after the `notes` functions so that we don't switch back and forth between styles too.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:797
+  LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
+  uintX_t addr; // Function address
+  // Struct representing the BBAddrMap information for one basic block.
----------------
Is there a specification to point to somewhere for this section? If so, what case style does it use? If there isn't, we should probably use LLVM style, though I'm not sure.


================
Comment at: llvm/lib/Object/ELF.cpp:379
+    uintX_t Address = static_cast<uintX_t>(Data.getAddress(Cur));
+    uint32_t NumBlocks = Data.getULEB128(Cur);
+    std::vector<typename Elf_BBAddrMap::BBEntry> BBEntries;
----------------
`getULEB128` can return `uint64_t`, so I'd use `uint64_t` here and below - it defers truncation of large values to as late as possible, which I think is a good thing. Optionally, also consider an error check that the values aren't greater than max UINT32. A ULEB128 has no defined max size, so if the input format is ULEB128, users might theoretically expect to allow more than 32-bits worth of data. Alternatively, consider using `uin64_t` for the struct member sizes. Values outside that range will be picked up by the ULEB parser and reported, so we don't need to do more then.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:1
+## This test checks how we handle the --elf-bb-addr-map option.
+
----------------
`--bb-addr-map` is the canonical option, so use that terminology in the comments rather than `--elf-...`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:60
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: llvm-readobj %t2.o --bb-addr-map 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=INVALID
----------------
grimar wrote:
> This needs a comment to describe what is exactly wrong with the section.
> There are multiple possible issues that might occur, but you test only one of them.
> 
> I'd suggest to create a valid section and use `ShSize` key with a macro to truncate it
> by different ways. This will allow to easily test each of cases
> 
> ```
>       uint32_t Offset = Data.getULEB128(Cur);
>       uint32_t Size = Data.getULEB128(Cur);
>       uint32_t Metadata = Data.getULEB128(Cur);
> ```
> 
> I.e. you can just reuse the first YAML:
> 
> 
> ```
> Sections:
>   - Name:    .llvm_bb_addr_map
>     Type:    SHT_LLVM_BB_ADDR_MAP
>     ShSize: [[SIZE=<none>]] <--- just add this line and use the power of macros
>     Entries:
> .....
> ```
> 
See also my out-of-line comment, but I think it would make more sense for the testing of the different ways things can go wrong to be in the libObject unit tests if possible, with just a single test in llvm-readobj lit tests showing that a warning is reported if there's a problem.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:373-374
+                          cl::desc("Display the BB address map section"));
+  cl::alias ELFBBAddrMap("elf-bb-addr-map", cl::desc("Alias for --bb-addr-map"),
+                         cl::aliasopt(BBAddrMap));
+
----------------
Any particular reason for the `elf` alias? The alias is there for some older options (cg-profile specifically) because they were initially added as `elf-...`, so we keep the alias for backwards compatibility. If there isn't a reason, I'd drop it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511