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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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))
----------------
make_unqiue


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:134
+
+  Extractor.getU8(C); // version
+  Extractor.getU8(C); // flags
----------------
Check whether the version is supported and add a test


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:156
+
+  StringsTable = Extractor.getData().slice(StrStart, StrEnd);
+
----------------
substr

delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:175
+
+  Extractor.getU8(C); // version
+  Extractor.getU8(C); // flags
----------------
Check whether the version is supported and add a test


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:210
+  while (C && C.tell() < LineInfoEnd) {
+    std::unique_ptr<BTFLinesVector> Lines(new BTFLinesVector());
+
----------------
make_unqiue


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:211
+    std::unique_ptr<BTFLinesVector> Lines(new BTFLinesVector());
+
+    auto SecNameOff = Extractor.getU32(C);
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:214
+    auto NumInfo = Extractor.getU32(C);
+
+    auto SecName = findString(SecNameOff);
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:217
+    auto Sec = Ctx.findSection(SecName);
+
+    for (uint32_t I = 0; C && I < NumInfo; ++I) {
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:220
+      auto RecStart = C.tell();
+
+      auto InsnOff = Extractor.getU32(C);
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:225
+      auto LineCol = Extractor.getU32(C);
+
+      Lines->push_back({InsnOff, FileNameOff, LineOff, LineCol});
----------------
delete blank line


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:287
+StringRef BTFParser::findString(uint32_t Offset) {
+  return StringsTable.slice(Offset, StringsTable.find(0, Offset));
+}
----------------
substr


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:2
+; REQUIRES: bpf-registered-target
+;
+; Verify that llvm-objdump can use .BTF.ext to extract line number
----------------
We don't generally add `^;$` lines. Having these lines actually makes `{` `}` jumping with Vim difficult...


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:3
+;
+; Verify that llvm-objdump can use .BTF.ext to extract line number
+; information in disassembly when DWARF is not available.
----------------
Some test directories, including llvm-objdump, prefer `;;` for non-RUN-non-CHECK comments. This makes comments stand out (may be highlit in some editors).


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:14
+; RUN: llc --mtriple bpfel %t.ll --filetype=obj -o %t
+; RUN: llvm-strip --strip-debug %t
+; RUN: llvm-objdump --section-headers %t | FileCheck --check-prefix=SECTIONS %s
----------------
`interleaved-source-test.ll` tests a BPF program with only BTF, but there is no test testing the behavior when both DWARF and BTF are present. We probably should dump the output of the original %t and the stripped %t into two text files, and check that the two text files are identical.


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:16
+; RUN: llvm-objdump --section-headers %t | FileCheck --check-prefix=SECTIONS %s
+; RUN: llvm-objdump --no-addresses --no-show-raw-insn -Sd %t | FileCheck %s
+;
----------------
Testing the addresses below, at least for one function, will be useful.


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:16
+; RUN: llvm-objdump --section-headers %t | FileCheck --check-prefix=SECTIONS %s
+; RUN: llvm-objdump --no-addresses --no-show-raw-insn -Sd %t | FileCheck %s
+;
----------------
MaskRay wrote:
> Testing the addresses below, at least for one function, will be useful.
You can simplify `-Sd` to `-S` as `-S` implies `-d`


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:27
+;
+;  clang -g -target bpf -emit-llvm -S ./Inputs/test.c
+;
----------------
Use `--target=bpf`.

I have sent https://lore.kernel.org/bpf/20230623020908.1410959-1-maskray@google.com/T/#u to replace `-target bpf` with `--target=bpf` and hopefully influenced new BPF users to migrate away from the deprecated `-target `


================
Comment at: llvm/test/tools/llvm-objdump/BPF/interleaved-source-test.ll:35
+; SECTIONS: .BTF.ext
+; SECTIONS-NOT: .debug_line
+; SECTIONS-NOT: .debug_line_str
----------------
This doesn't check that `.debug_info` is not above `.BTF`.

`FileCheck %s --implicit-check-not=.debug_` may be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149058