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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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"
----------------
Macros in public headers should be used with caution as they may collide with downstream users.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:308
+
+struct IntType : CommonType {
+  DEFINE_TAIL(uint32_t, getBitsInfo)
----------------
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.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:385
+// add standard layout assertions just in case.
+static_assert(std::is_standard_layout<BTF::CommonType>());
+static_assert(std::is_standard_layout<BTF::IntType>());
----------------
Unneeded if we don't define these classes.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:109
+  struct ParseOptions {
+    bool LoadLines;
+    bool LoadTypes;
----------------
use member initializers, otherwise it's error-prone (without `{}` or ` = {}`, we get uninitialized values).


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


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:203
+
+  TypesBuffer = OwningArrayRef<uint8_t>(RawData.size());
+  std::memcpy(TypesBuffer.data(), RawData.data(), RawData.size());
----------------
Use the `OwningArrayRef(ArrayRef<T> Data)` constructor to avoid a separate memcpy


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:211
+
+  // void
+  Types.push_back(&VoidTypeInst);
----------------



================
Comment at: llvm/test/tools/llvm-objdump/BPF/core-relo-formatting.s:1
+# REQUIRES: bpf-registered-target
+
----------------
We probably should change one of the files to test both `-d` and `-dr`, so that the effect of `-r` is clearer.


================
Comment at: llvm/test/tools/llvm-objdump/BPF/core-relo-formatting.s:53
+# ADDR:       1:	r1 = 0x1
+# ADDR:		0000000000000008:  CO-RE <type_exists> [3] struct foo
+# ADDR:       2:	call -0x1
----------------
Use `-NEXT:` whenever applicable https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive https://maskray.me/blog/2021-08-08-toolchain-testing#use-the-check-next-directive (I decided to add a section to this article so that I don't need to repeat the argument for future reviews:)

ditto throughout 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1487
+  if (InlineRelocs && BTFParser::hasBTFSections(Obj)) {
+    BTF.reset(new llvm::BTFParser());
+    BTFParser::ParseOptions Opts = {};
----------------
`BTF = std::make_unique<BTFParser>();`

However, `std::optional<BTFParser> BTF` and `BTF.emplace()` is better as we avoid an unneeded dynamic allocation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150079