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

Mon Sep 4 12:31:18 PDT 2023

eddyz87 added a comment.

Hi @MaskRay, @jhenderson,

Thank you for the feedback, I think I covered all but one items.
The last remaining item is to verify that unit test is working on the bigendian system.
I'll update the revision once I check that.

================
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;
----------------
jhenderson wrote:
> 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).
Changed to MiB.

================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:205
+                                StringRef RawData) {
+  using support::big;
+  using support::endianness;
----------------
MaskRay wrote:
> Some using declarations such as big,little, seem unnecessary?
`support::endian::system_endianness` was unused, indeed. The `big` and `little` are used in the following declaration:

```
lang=cpp
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 << "<";
----------------
jhenderson wrote:
> `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.
Changed to `reloc` in identifiers and "relocation" in strings.

================
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].
----------------
jhenderson wrote:
> 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?
Removed.

================
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`.
----------------
jhenderson wrote:
> "splits" is duplicated.
Right, sorry about that.

================
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,
----------------
eddyz87 wrote:
> yonghong-song wrote:
> > eddyz87 wrote:
> > > 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.
> > @eddyz87 Let us add relocation documentation in https://github.com/torvalds/linux/blob/master/Documentation/bpf/llvm_reloc.rst
> > and add a reference in https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-ext-section inst
> > Thanks!
> Working on it, submitted patch [[ https://lore.kernel.org/bpf/20230824230102.2117902-1-eddyz87@gmail.com/ | here ]].
> 
Documentation was merged to bpf tree: [[ https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf/+/refs/heads/master/Documentation/bpf/llvm_reloc.rst#247 | here ]] and [[ https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf/+/refs/heads/master/Documentation/bpf/btf.rst#795 | here ]].

================
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
----------------
jhenderson wrote:
> 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.
Updated to `| \`. I find the version with `|` at the new line a bit more aesthetically pleasing, but oh-well.

================
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}});
----------------
jhenderson wrote:
> `std::numeric_limits::max<uint32_t>()` might be clearer?
[[ https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-kind-decl-tag | Documentation ]] refers to value `-1` (for unsigned field :) ), so I think it's better to keep `-1` here to avoid confusion.

================
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 */);
----------------
jhenderson wrote:
> 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.
Thanks, replaced with local macro:

```
lang=cpp
#define ASSERT_SUCCEEDED(E) ASSERT_THAT_ERROR((E), Succeeded())
```

I do use `FailedWithMessage` in macro `EXPECT_PARSE_ERROR`.

================
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});
----------------
jhenderson wrote:
> Nit (also below for other comments).
Added dots for comments that look like English sentences, did not add dots for comments that refer to type definitions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150079