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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thank you for the update. The code looks good, i've only got a few nits and test suggestions.



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFContext.h:1
+//===- BTFContext.h -------------------------------------------------------===//
+//
----------------
See https://llvm.org/docs/CodingStandards.html#file-headers

This needs a `C++` marker. Ditto elsewhere


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:44
+  // Each BTFLinesVector is sorted by `InsnOffset` to allow fast lookups.
+  DenseMap<uint64_t, std::unique_ptr<BTFLinesVector>> SectionLines;
+
----------------
We can remove the unique_ptr indirection: `DenseMap<uint64_t, BTFLinesVector> SectionLines;`

If you change BTFLinesVector to use `SmallVector<xxx, 0>`, it will be even smaller.


================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:55
+  // Offset is relative to string table start.
+  StringRef findString(uint32_t Offset);
+
----------------



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:62
+  // owned by this class.
+  BTF::BPFLineInfo *findLineInfo(SectionedAddress Address);
+
----------------



================
Comment at: llvm/include/llvm/DebugInfo/BTF/BTFParser.h:75
+
+  // Returns true if `Obj` has .BTF and .BTF.ext sections
+  static bool hasBTFSections(const ObjectFile &Obj);
----------------



================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:53
+                                      DILineInfoSpecifier Specifier) {
+  // BTF does not convey such information
+  return {};
----------------



================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:65
+                   std::function<void(Error)> ErrorHandler) {
+  auto Ctx = std::unique_ptr<BTFContext>(new BTFContext());
+  if (Error E = Ctx->BTF.parse(Obj))
----------------
Make the constructor public so that `make_unique` can be used.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFContext.cpp:77
+                   std::function<void(Error)> ErrorHandler) {
+  auto Ctx = std::unique_ptr<BTFContext>(new BTFContext());
+  if (auto E = Ctx->BTF.parse(Obj))
----------------
nickdesaulniers wrote:
> eddyz87 wrote:
> > MaskRay wrote:
> > > make_unqiue
> > I defined `BTFContext` constructor as private and clang disallows me to call private constructor here:
> > 
> > ```
> > unique_ptr.h:1065:34: error: calling a private constructor of class 'llvm::BTFContext'
> >     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
> > ```
> > 
> > If you strictly disprove usage of `unique_ptr()` I can make the constructor public.
> right, std::make_unique requires the constructor to be private, unfortunately. I think the code LGTM as is.
Make the constructor public so that make_unique can be used.

DWARFContext has a public constructor as well.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:102
+  std::optional<SectionRef> findSection(StringRef Name) {
+    auto It = Sections.find(Name);
+    if (It != Sections.end())
----------------
MaskRay wrote:
> `return Sections.lookup(Name);`
Sorry, `lookup` cannot be used. The existing code is correct.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:132
+    return Err("unexpected .BTF header length: ") << HdrLen;
+  Extractor.getU32(C); // type_off
+  Extractor.getU32(C); // type_len
----------------
Make it explicit you discard the return value.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:165
+    return Err("unsupported .BTF.ext version: ") << (unsigned)Version;
+  Extractor.getU8(C); // flags
+  uint32_t HdrLen = Extractor.getU32(C);
----------------
Make it explicit you discard the return value.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:171
+    return Err("unexpected .BTF.ext header length: ") << HdrLen;
+  Extractor.getU32(C); // func_info_off
+  Extractor.getU32(C); // func_info_len
----------------
Make it explicit you discard the return value.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:207
+      Lines->push_back({InsnOff, FileNameOff, LineOff, LineCol});
+      C.seek(RecStart + RecSize);
+    }
----------------
If `C` has errored, we probably should not invoke `Lines->push_back({InsnOff, FileNameOff, LineOff, LineCol});`


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:209
+    }
+    std::sort(Lines->begin(), Lines->end(),
+              [](const BTF::BPFLineInfo &L, const BTF::BPFLineInfo &R) {
----------------
`llvm::stable_sort(*Lines, ...)` to make the behavior deterministic no matter what `std::sort `implementations are unused.
See https://libcxx.llvm.org/DesignDocs/UnspecifiedBehaviorRandomization.html


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:271
+  auto LineInfo =
+      std::lower_bound(SecInfo->begin(), SecInfo->end(), Address.Address,
+                       [](const BTF::BPFLineInfo &Line, uint64_t Addr) {
----------------
When lower_bound is used with a custom predicate, it's usually better to use https://en.cppreference.com/w/cpp/algorithm/partition_point instead


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:225
+      auto LineCol = Extractor.getU32(C);
+
+      Lines->push_back({InsnOff, FileNameOff, LineOff, LineCol});
----------------
MaskRay wrote:
> delete blank line
I think `init` (a simple method) can be inlined into `parse`, then we may even avoid the two `Ctx.findSection(BTF...Name);` calls as we can check the section name in the section iteration loop.

In addition, the new code will be more similar to hasBTFSections


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:18
+
+using namespace llvm;
+using namespace object;
----------------
We normally place `using namespace` immediately below `#include`


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:26
+
+raw_ostream &operator<<(raw_ostream &OS, const yaml::BinaryRef &Ref) {
+  Ref.writeAsHex(OS);
----------------
Use static instead of an anonymous namespace. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:53
+
+struct MockData1 {
+  struct B {
----------------
Consider starting the anonymous namespace here, including all TEST( below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149058