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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Initial code review pass



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:33
+
+using BTFLinesVector = std::vector<BTF::BPFLineInfo>;
+
----------------
IIRC, I think you can move this `using` statement into the class definition to scope its use within the class? Otherwise `using` in headers makes me nervous.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:44
+  // Each BTFLinesVector is sorted by `InsnOffset` to allow fast lookups.
+  std::map<uint64_t, std::unique_ptr<BTFLinesVector>> SectionLines;
+
----------------
why `std::map` over something like `llvm::DenseMap`?
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:30-33
+void BTFContext::dump(raw_ostream &OS, DIDumpOptions DumpOpts) {
+  // This function is called from objdump when --dwarf=? option is set.
+  // BTF is no DWARF, so ignore this operation for now.
+}
----------------
`dump` is super common for most classes in LLVM when debugging. If you're not going to provide a meaningful definition, I think you can do so in the header file.  Same for the constructor above.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:37
+                                             DILineInfoSpecifier Specifier) {
+  auto *LineInfo = BTF.findLineInfo(Address);
+  if (!LineInfo)
----------------
plz no auto


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:39-41
+    return {};
+
+  DILineInfo Result;
----------------
pretty sure this will prevent NVRO. Just declare Result first, then always return it.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:51
+  // BTF does not convey such information
+  return DILineInfo();
+}
----------------
here you can `return {};`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:59
+  // JITEventListener implementations. Ignore it for now.
+  return DILineInfoTable();
+}
----------------
here you can `return {};`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:66-68
+  return DIInliningInfo();
+}
+
----------------
here you can `return {};`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:71
+  // BTF does not convey such information
+  return std::vector<DILocal>();
+}
----------------
probably here you can `return {};`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:78
+  auto Ctx = std::unique_ptr<BTFContext>(new BTFContext());
+  if (auto E = Ctx->BTF.parse(Obj))
+    ErrorHandler(std::move(E));
----------------
plz no auto here


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:104
+  Expected<DataExtractor> makeExtractor(SectionRef Sec) {
+    auto Contents = Sec.getContents();
+    if (!Contents)
----------------
plz no auto here

plz review https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable and update the remainder of this patch where appropriate.


================
Comment at: llvm/tools/llvm-objdump/SourcePrinter.h:154-155
 
+  std::optional<StringRef> getLine(const DILineInfo &LineInfo,
+                                   StringRef ObjectFilename);
+
----------------
How about returning just a `StringRef`; in the case of failure, return a `StringRef` that is empty, such as a default constructed StringRef.


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:23
+#include <memory>
+#include <optional>
+
----------------
unused?


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:25
+
+#define __packed __attribute__((packed))
+#define LC(Line, Col) ((Line << 10u) | Col)
----------------
unused?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149058