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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
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
----------------
jhenderson wrote:
> rahmanl wrote:
> > 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. 
> 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.
Thanks James. Allow me get to this (and the cmd documentation) after I get the logic right.
Would you mind elaborate on what specifically needs to be checked in the libObject unit test (Also, is there a precedent/example I can follow)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511