[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.

Note to self: I've not yet reviewed the testing in depth. Need to do that still.

> 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)?

The existing libObject unit tests are patchy at best, unfortunately, so finding a clear-cut precedent is a little tricky. Probably the easiest thing to do would be to follow things like the `InvalidSymbolTest` in `ELFObjectFileTest.cpp` to generate an `ELFFile` instance from YAML written as a string in the code. With a bit of care, you could probably dynamically build up the YAML string from common components to avoid a verbose string literal in each test case. Once you have an `ELFFile` instance, you can then just call `decodeBBAddrMap` on that. The aim of this test would be to test the code that is in the library directly, without worring about whatever llvm-readobj does. In particular, you can test all the error paths there, using things like `EXPECT_THAT_EXPECTED(..., FailedWithMessage(...));`. In the lit test, you could then treat the function as a black box and test only one way of getting the error (to show that the error is handled properly), plus that the dumpign code works as expected in a valid case. Does that make sense?



================
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.
----------------
Nit: these comments probably all want trailing full stops.


================
Comment at: llvm/lib/Object/ELF.cpp:620
+  Error ULEBSizeErr = Error::success();
+  auto readULEB128AsUInt32 = [&Data, &Cur, &ULEBSizeErr]() -> uint32_t {
+    if (ULEBSizeErr)
----------------
Lambda case style is the same as variable case style (i.e. UpperCamelCase). I guess, because they're more like pointers to functions than functions themselves?


================
Comment at: llvm/lib/Object/ELF.cpp:627
+          "ULEB128 value at offset 0x" + Twine::utohexstr(Cur.tell()) +
+          " exceeds UINT32_MAX (0x" + Twine::utohexstr(Value) + ").");
+    return static_cast<uint32_t>(Value);
----------------
Coding standard for error messages is no trailing full stop (for consistency with most existing messages).


================
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
----------------
rahmanl wrote:
> 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)?
See out-of-line comment (moved out-of-line due to length).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511