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

[Eduard Zingerman via Phabricator via llvm-commits]

eddyz87 added a comment.

Hi @nickdesaulniers , @MaskRay ,

Thank you for taking a look at this revision.
I've added requested changes, and put inline comments for a few points I don't agree with.
Could you please take a second look?

P.S.
A silly question, who has to check the "Done" box for sub-discussions, author or reviewer?



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


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:55
+      : Buffer(), Stream(Buffer) {
+    *this << "Error while reading " << SectionName
+          << " section: " << C.takeError();
----------------
MaskRay wrote:
> don't capitalize. see https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Done under assumption that you refer to the error message, removed capitalization from all error messages.


================
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
----------------
MaskRay wrote:
> The "The section name to SectionRef mapping" should be attached to `Sections`, not the class.
The purpose of the comment was to explain the purpose of this class existence.
I changed comment to focus on the class itself.


================
Comment at: llvm/lib/DebugInfo/BTF/BTFParser.cpp:287
+StringRef BTFParser::findString(uint32_t Offset) {
+  return StringsTable.slice(Offset, StringsTable.find(0, Offset));
+}
----------------
MaskRay wrote:
> substr
- `substr(size_t Start, size_t N)` -> substring from [Start, Start + N)
- `slice(size_t Start, size_t End)` -> substring from [Start, End)

Call to `StringsTable.find(0, Offset)` returns the end position for string starting at `Offset`. Why is `substr()` better in this context?


================
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
----------------
MaskRay wrote:
> `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.
I've added `llvm-objdump ... %t | FileCheck %s` step before DWARF information stripping, so objdump output is verified for both DWARF-BTF and BTF-only versions of the object file.


================
Comment at: llvm/tools/llvm-objdump/SourcePrinter.h:154-155
 
+  std::optional<StringRef> getLine(const DILineInfo &LineInfo,
+                                   StringRef ObjectFilename);
+
----------------
nickdesaulniers wrote:
> How about returning just a `StringRef`; in the case of failure, return a `StringRef` that is empty, such as a default constructed StringRef.
This is probably fine and no tests fail if I do such change.
However, it implies that `SourcePrinter::printSources()` can never print an empty line.
The original `SourcePrinter::printSources()` does not have such property, idk if this is important.
I included this change but I think `std::optional` is better because it highlights that return value is not mandatory and makes the "absent" value indicator self-explanatory.


================
Comment at: llvm/unittests/DebugInfo/BTF/BTFParserTest.cpp:90
+    struct {
+      uint32_t LineRecSize = sizeof(BTF::BPFLineInfo);
+      struct {
----------------
nickdesaulniers wrote:
> `size_t`
This is a part of the packed structure with predefined layout, the `u32` value is expected in this position.

Note: structure `MockData1` is used as a blob to be parsed by BTF parser in the tests below, its layout has to match BTF binary encoding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149058