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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thanks for the comments @grimar



================
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:
> .....
> ```
> 
Thanks for the suggestion. Adopted.
I am not sure if I want to exercise more invalid cases. Of course I can truncate the content by different sizes, but I don't think it will add a lot of value. 


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:62
+# RUN: llvm-readobj %t2.o --bb-addr-map 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=INVALID
+# INVALID: warning: '[[FILE]]': unable to dump the SHT_LLVM_BB_ADDR_MAP section: unable to decode LEB128 at offset 0x{{([[:xdigit:]]{8})}}: malformed uleb128, extends past end
+--- !ELF
----------------
grimar wrote:
> I am not sure that `0x{{([[:xdigit:]]{8})}}` is usefull? I'd just check the offset.
> Also I guess you can reuse this check line for other broken cases and have a `[[OFFSET]]` macro instead.
> 
> 
> ```
>  ...  | FileCheck %s -DFILE=%t2.o --check-prefix=INVALID -DOFFSET=0x....
>  # INVALID: warning: '[[FILE]]': unable to dump the SHT_LLVM_BB_ADDR_MAP section: unable to decode LEB128 at offset [[OFFSET]]: malformed uleb128, extends past end
> ```
> 
> 
Thanks. Used `[[OFFSET]]` even though it's only used for one test case.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:71
+    Type:    SHT_LLVM_BB_ADDR_MAP
+    Size:    8
----------------
grimar wrote:
> Usually we reduce the number of spaces to minimal.
> 
> ```
>   - Name: .llvm_bb_addr_map
>     Type: SHT_LLVM_BB_ADDR_MAP
>     Size: 8
> ```
> 
> (The same applies for the first YAML).
I removed this YAML since I am reusing the first YAML for the invalid case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511