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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I did a quick skim of comments and the like, but I don't really have the time to read up on BTF, so can't contirbute anything more meaningful really, sorry.



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:101
 
+// Constants for CommonType::Info field
+enum : uint32_t { FWD_UNION_FLAG = (1u << 31), ENUM_SIGNED_FLAG = (1u << 31) };
----------------
Nit.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTF.h:279
 
+/// CO-RE relocation kind codes used in .BTF.ext section
+enum PatchableRelocKind : uint32_t {
----------------
Nit.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:46-47
+  // A copy of types table from the object file but using native byte
+  // order. Should not be too big in practice, e.g. for ~250Mb vmlinux
+  // image it is ~4Mb.
+  OwningArrayRef<uint8_t> TypesBuffer;
----------------
The "Mb" suffix is ambiguous/potentially incorrect. Is it meant to be megabits (I doubt it)?

Assuming it is intended to be binary megabytes (i.e. 1024*1024 bytes), consider using "MiB" (mebibytes).


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:107
+
+  // Allow to selectively load BTF information
+  struct ParseOptions {
----------------
Nit.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:213
+      ArrayRef<uint8_t>((const uint8_t *)RawData.data(), RawData.size()));
+  // Switch endianness if necessary
+  endianness Endianness = Ctx.Obj.isLittleEndian() ? little : big;
----------------



================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:555
+
+static void reloKindName(uint32_t X, raw_ostream &Out) {
+  Out << "<";
----------------
`reclocKindName` maybe? ("reloc" is a more common abbreviation for "relocation").

Same goes below at line 559 ("relo kind #") and at various other places (e.g. `ReloKindGroup`).

Neither of these apply if "relo" is an abbreviation used in the BTF spec or similar.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:607-608
+// from the following files:
+// - llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
+// - llvm/lib/Target/BPF/BTFDebug.cpp
+// And than processed by libbpf's BPF program loader [1].
----------------
I'm a little nervous about referencing file paths in comments - I would expect them to fairly easily rot. Do you actually need to do that?


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:674
+  // Relocation access string follows pattern [0-9]+(:[0-9]+)*,
+  // e.g.: 12:22:3. Code below splits splits `SpecStr` by ':',
+  // parses numbers, and pushes them to `RawSpec`.
----------------
"splits" is duplicated.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:689
+
+  // Print relocation kind to `Stream`
+  reloKindName(Reloc->RelocKind, Stream);
----------------



================
Comment at: llvm/test/tools/llvm-objdump/BPF/core-relo-byte-offset.ll:5-7
+; RUN: llc --mtriple bpfel %s --filetype=obj -o - \
+; RUN:  | llvm-objdump --no-addresses --no-show-raw-insn -dr - \
+; RUN:  | FileCheck %s
----------------
I have a personal preference to put the `|` at the end of the previous line. My reasoning is that by looking at any given line, you can tell if the command itself is continued (it ends with `| \` or simply `\`), without needing to look at the next line, whilst the continuation lines are clearly continuations (due to being indented), and will either start with an executable name (indicating they are the start of a new command) or otherwise (indicating that they aren't).

Inline edit is to demonstrate - not a requirement if you object to the proposal.


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:565
+  D.addType({D.addString("18"), mkInfo(BTF::BTF_KIND_DECL_TAG), {0}});
+  D.addTail((uint32_t)-1);
+  D.addType({D.addString("19"), mkInfo(BTF::BTF_KIND_TYPE_TAG), {0}});
----------------
`std::numeric_limits::max<uint32_t>()` might be clearer?


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:596-597
+  BTFParser BTF;
+  Error Err = BTF.parse(D.makeObj());
+  ASSERT_FALSE(Err);
+  ASSERT_EQ(BTF.typesCount(), 4u /* +1 for void */);
----------------
Similar can be done in various other places. There's also `EXPECT` variations and an `ASSERT_THAT_EXPECTED` for Expected types, and you can use `FailedWithMessage()` to check that the error failed with a specific error message too.


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:664
+  D.addRelocSec({D.addString("foo"), 7});
+  // List of all possible correct type relocations for type id #1
+  D.addReloc({0, 1, Zero, BTF::BTF_TYPE_ID_LOCAL});
----------------
Nit (also below for other comments).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150079