[PATCH] D150079: [BPF][DebugInfo] Show CO-RE relocations in llvm-objdump

[Eduard Zingerman via Phabricator via llvm-commits]

eddyz87 added a comment.

In D150079#4608909 <https://reviews.llvm.org/D150079#4608909>, @MaskRay wrote:

> Sorry, was pretty busy handling a backlog of reviews and day work... Feel free to ping after a week https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted

Thanks a lot for taking a look.

> In a `-DBUILD_SHARED_LIBS=on` build, I get `ld.lld: error: undefined symbol: llvm::BTFParser::parse(llvm::object::ObjectFile const&, llvm::BTFParser::ParseOptions const&)`. We need
>
>   --- i/llvm/tools/llvm-objdump/CMakeLists.txt
>   +++ w/llvm/tools/llvm-objdump/CMakeLists.txt
>   @@ -5,2 +5,3 @@ set(LLVM_LINK_COMPONENTS
>      BinaryFormat
>   +  DebugInfoBTF
>      DebugInfoDWARF

Done

> https://github.com/MaskRay/Config/wiki/LLVM#build-modes Personally, when I make CMake changes, I'll check both the default config and `-DBUILD_SHARED_LIBS=on`

Noted, thank you.



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:280
+enum PatchableRelocKind : uint32_t {
+#define HANDLE_RELO_KIND(Val, Name) Name = Val,
+#include "CoreRelo.def"
----------------
MaskRay wrote:
> Macros in public headers should be used with caution as they may collide with downstream users.
Removed CoreRelo.def, I reused it for printable names in the original revision but it is changed since then, so there is no need in this file.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:308
+
+struct IntType : CommonType {
+  DEFINE_TAIL(uint32_t, getBitsInfo)
----------------
MaskRay wrote:
> These derived classes are only referenced in `byteSize`. This doesn't justify creating so many derived classes.
> 
> See that `printMod` you only use the kind value, not the derived classes.
Those are all types that have trailing items. I removed the types not used outside of `byteSize` and switched `byteSize` to use switch over kind (which is a bit less safe, imo).

As a final step for BTF support in objdump Yonghong wanted me to add support for printing BTF definitions like `bpftool btf dump file` does. The definitions in question would have been useful in that case. But I can add them back when necessary.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:124
+  Error parse(const ObjectFile &Obj,
+              const ParseOptions &Opts = {true, true, true});
 
----------------
MaskRay wrote:
> avoid default argument. this doesn't 
Replaced with overload.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:203
+
+  TypesBuffer = OwningArrayRef<uint8_t>(RawData.size());
+  std::memcpy(TypesBuffer.data(), RawData.data(), RawData.size());
----------------
MaskRay wrote:
> Use the `OwningArrayRef(ArrayRef<T> Data)` constructor to avoid a separate memcpy
Done, but it is kind-off ugly because `StringRef` encapsulates `char` and I use `uint8_t` for `TypesBuffer`:

```
lang=cpp
  TypesBuffer = OwningArrayRef<uint8_t>(
    ArrayRef<uint8_t>((const uint8_t *)RawData.data(), RawData.size()));
```


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:332
 
-Error BTFParser::parse(const ObjectFile &Obj) {
+Error BTFParser::parseRelocInfo(ParseContext &Ctx, DataExtractor &Extractor,
+                                uint64_t RelocInfoStart,
----------------
MaskRay wrote:
> @ast @yonghong-song BTF relocations seem undocumented on https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-ext-section ?
I will add necessary documentation.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:674
+
+    if (SpecStr.size() && SpecStr[0] != ':')
+      return Fail(format("unexpected spec string delimiter: '%c'", SpecStr[0]));
----------------
MaskRay wrote:
> What if SpecStr is empty?
> 
> https://en.cppreference.com/w/cpp/string/basic_string_view/substr I think we may harden StringRef::substr to abort when pos > size in the future.
Changed to exit loop if `SpecStr` is empty after `consumeUnsignedInteger`, added missing check that type relocation spec should be '0'.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:700
+    if (!NextType)
+      return Fail(format("unknown type id: %d in modifiers chain", CurId));
+    Type = NextType;
----------------
MaskRay wrote:
> This is untested
Missed this one :-/


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:530
+
+  // won't work on big-endian machine?
+  D.addType({D.addString("1"), KIND(BTF::BTF_KIND_INT), {4}});
----------------
MaskRay wrote:
> If we are unsure whether this works on a big-endian machine, we can ask someone to verify when this patch is about to land...
I'll try to check in the VM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150079