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

In D95511#2541120 <https://reviews.llvm.org/D95511#2541120>, @rahmanl wrote:

> In D95511#2541083 <https://reviews.llvm.org/D95511#2541083>, @mtrofin wrote:
>
>> In D95511#2541073 <https://reviews.llvm.org/D95511#2541073>, @rahmanl wrote:
>>
>>> In D95511#2541040 <https://reviews.llvm.org/D95511#2541040>, @mtrofin wrote:
>>>
>>>> Can the output be consumed programmatically?  (i.e. is it for example valid yaml or json)
>>>
>>> Yes, but it's not yaml or json. The output is `std::vector<Elf_BBAddrMap>` and it is accessible via the library call: `ELFFile::decodeBBAddrMap(Elf_Shdr&)` (However, the user needs to find the BB address map section).
>>
>> I meant the textual output. It's very close to json - the benefit of it actually being json is that one could script from llvm-readobj output.
>
> I see. You're right. The output is similar to json. This is produced by `ScopedPrinter`. The relevant sources do not point out similarity with JSON at all. Maybe @jhenderson can answer this.

The LLVM-style output format for llvm-readobj is not intended to be JSON, although it is somewhat similar. Primarily, the output is intended for ease of testing within the LLVM lit tests. See in particular the second of the following two commits (the first contains some general rules for the output format).

https://github.com/llvm/llvm-project/commit/9cad53cfeceed2ef75aa17f0553edd27afd2e950
https://github.com/llvm/llvm-project/commit/d7e7003e8b2d4632b72533e0d05ad5558e5bdcbc#diff-53e1a2a2a247ad1a4cac5242c17c0aa36eefdec70b4369fb31f911aaf89aad2c

I don't think it's practical to make the general output JSON-like due to the amount of turbulence this would cause. If there's a particularly strong desire to do this, it should be discussed on llvm-dev in the first instance, I think, and secondly, I think would be best as a third output format (e.g. --elf-output-style=json).



================
Comment at: llvm/docs/CommandGuide/llvm-readelf.rst:37
+
+ Display the contents of the BB address map section(s), which contains the
+ address of each function, along with the relative offset of its basic blocks.
----------------
Spell out "BB". I assume it is "basic block"? Also, as this is only implemented for LLVM output style, I'd mention that in the documentation. Also, you say "section(s)", implying there might be more than one. However, the code only supports dumping a single section per object file. Either your code or your documentation needs updating!


================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:153
+
+ Display the contents of the BB address map section(s), which contains the
+ address of each function, along with the relative offset of its basic blocks.
----------------
Same as above.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:800
+  struct BBEntry {
+    uint32_t Offset;   // Offset of basic block relative to function begin.
+    uint32_t Size;     // Size of the basic block.
----------------



================
Comment at: llvm/lib/Object/ELF.cpp:643-644
+  }
+  if (ULEBSizeErr)
+    return std::move(ULEBSizeErr);
+  if (!Cur)
----------------
I might be mistaken, but won't this cause an assertion due to `Cur` not being checked //and reported// if it is in a bad state? I think it could happen if reading `NumBlocks` produces too large a number, and then a later value is truncated (for example).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:17
+
+## Check that we can detect when enoded BB entry fields exceed UINT32 limits.
+# RUN: yaml2obj %s -DBITS=64 -DBBADDRESSOFFSET=0xFFFFFFFF4 -o %t3.o
----------------
But actually, you only need one of this and the above test case that checks the error behaviour in a lit test - the libObject unit test is sufficient for the other failure path(s). In fact, you should probably change the comment of the kept error case, simply to say something like "Check that a malformed section can be handled."


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:515
+    ASSERT_THAT_EXPECTED(BBAddrMapSecOrErr, Succeeded());
+    ASSERT_EQ((*BBAddrMapSecOrErr)->sh_type, ELF::SHT_LLVM_BB_ADDR_MAP);
+    EXPECT_THAT_ERROR(Elf.decodeBBAddrMap(**BBAddrMapSecOrErr).takeError(),
----------------
I'm not sure this line is really needed?


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:529
+
+  // Check that we can detect when the enoded BB entry fields exceed the UINT32
+  // limit.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511