[PATCH] D149058: [BPF][DebugInfo] Use .BPF.ext for line info when DWARF is not available

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:29
+#define DEBUG_TYPE "debug-info-btf-parser"
+#define BTF_SECTION_NAME ".BTF"
+#define BTF_EXT_SECTION_NAME ".BTF.ext"
----------------
`const char BTF_SECTION_NAME[] = ".BTF"`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:47
+//
+class Err {
+  std::string Buffer;
----------------
See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

This dangerously leaves a globally named `Err` which may collide with user programs if they link in LLVM.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:52
+public:
+  Err(const char *InitialMsg) : Buffer(InitialMsg), Stream(Buffer){};
+  Err(const char *SectionName, DataExtractor::Cursor &C)
----------------
`{};` => ` {}`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:55
+      : Buffer(), Stream(Buffer) {
+    *this << "Error while reading " << SectionName
+          << " section: " << C.takeError();
----------------
don't capitalize. see https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:80
+
+// The section name to SectionRef mapping is only needed while
+// ObjectFile is parsed, it is not needed for queries against
----------------
The "The section name to SectionRef mapping" should be attached to `Sections`, not the class.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:86
+  const ObjectFile &Obj;
+  std::map<StringRef, SectionRef> Sections;
+
----------------
map is slow. Here we don't need a stable iteration order. Use `DenseMap<StringRef, SectionRef>`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:250
+  ParseContext Ctx(Obj);
+
+  if (auto E = Ctx.init())
----------------
Nit: delete blank line.

This is not the Linux kernel community. Generally we don't add a blank line after a variable declaration. IMHO it often unnecessarily increases the number of lines without improving readability.

You may want to fix this elsewhere in the patch.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:251
+
+  if (auto E = Ctx.init())
+    return E;
----------------
`Error E`

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable "Don’t “almost always” use auto"


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:256
+  auto BTFExt = Ctx.findSection(BTF_EXT_SECTION_NAME);
+
+  if (!BTF)
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:258
+  if (!BTF)
+    return Err("Can't find .BTF section");
+
----------------
don't capitalize


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:259
+    return Err("Can't find .BTF section");
+
+  if (!BTFExt)
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:276
+  for (auto Sec : Obj.sections()) {
+    if (auto Name = Sec.getName()) {
+      HasBTF |= *Name == BTF_SECTION_NAME;
----------------
In `LLVM_ENABLE_ABI_BREAKING_CHECKS` mode, in case of an error, this `Expected` return value will crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149058