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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a subscriber: ast.
MaskRay added inline comments.


================
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,
----------------
@ast @yonghong-song BTF relocations seem undocumented on https://www.kernel.org/doc/html/latest/bpf/btf.html#btf-ext-section ?


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

splits `SpecStr` by ':',  parses numbers, and pushes them to `RawSpec`.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:674
+
+    if (SpecStr.size() && SpecStr[0] != ':')
+      return Fail(format("unexpected spec string delimiter: '%c'", SpecStr[0]));
----------------
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.


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



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


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:703
+  }
+  // Print the type name to `Stream`
+  if (CurId == 0) {
----------------



================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:736
+  ReloKindGroup Group = reloKindGroup(Reloc);
+
+  // Type-based relocations don't use access string.
----------------
Unlike Linux kernel, we don't keep a blank line after a variable declaration.
Group is immediately used so not having a blank line is arguably more readable.


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:358
 
+#define KIND(Kind) ((Kind) << 24)
+
----------------
This macro may collide with others (e.g. `llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h`). Use a static function


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:402
+  void reset() {
+    ObjStorage.resize(0);
+    Types.resize(0);
----------------
clear is more idiomatic


================
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}});
----------------
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...


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:622
+// Shorter name for initializers below
+typedef SectionedAddress SA;
+
----------------
C++ prefers `using`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150079