Add support for DWARF embedded source to llvm-symbolizer

[aorlov]

Summary

This patch adds DWARF embedded source printout to llvm-symbolizer.

[dblaikie]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

68–71

Could you pull this (& any other refactoring you think makes the semantic-changing patch more isolated/focussed) into a preliminary commit?

259–276

I take it this is trimming the trailing lines that won't be needed, while leaving the leading lines so the algorithm in printContext can still retrieve the lines in a source-agnostic way (so it can retrieve them in the same way from embedded source as non-embedded source)

It seems unfortunate to have split the pruning logic in two places like this - perhaps a generic function that takes the line iterator & just the desired context (pruning both leading and trailing lines) - then use that function here, and have the "Source" attribute contain the fully pruned text.

Then in printContext if the Source value is provided, use that literally - otherwise open the file and prune its leading/trailing using the same function?

(also that refactoring - pulling out such a reusable pruning function - should be done as a separate commit)

[aorlov]

https://reviews.llvm.org/D102539

[dblaikie]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

263

It's not valid to dereference an end iterator like this.

What is this code intended to address?

270–279

This looks rather similar to PlainPrinterBase::printContext - perhaps it could be refactored out into a common function?

[aorlov]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

263

Note llvm::line_iterator is badly designed.
It is checks *Pos == '\0' in many places instead of comparing Pos with Buffer.end().
It requires '\0' at the end of buffer which is not necessary actually.
Look at an assert on llvm/lib/Support/LineIterator.cpp, line 47.

270–279

Correct. I copied the Source context to JSON as is in the first implementation. Then I decided to format it too. I have moved the code to the common function format().

[aorlov]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

263

Note the current implementation of line_iterator can be used with the full source, but not with the pruned source if it didn't coincide with the end of the source file.

[dblaikie]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

40

This still isn't valid - there's no guarantee that end is dereferenceable. (using [] doesn't make the code any safer/different - it's still a dereference)

263

Either the code will need to unconditionally copy the StringRef into a std::string (in which case, maybe it'd be simpler if the SourceCode structure stored the data in a std::string in the first place? then it wouldn't need the MemoryBuffer member either, since it wouldn't be holding live pointers to the memory mapped file) - or the line_iterator would need to be fixed or a different tool used - which, maybe that's the thing to do? If the original code that extracted the relevant lines already used the line iterator (so features like blank line skipping, etc, have already been handled) - maybe this code could do something simpler, like splitting on newlines, rather than using the full line iterator?

[aorlov]

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

40

I just used the same checking as llvm/lib/Support/LineIterator.cpp, line 47:

assert(Buffer.getBufferEnd()[0] == '\0');

Any suggestions?

263

Note currently line_iterator does not skip blank lines.
I will prepare an own implementation without llvm::line_iterator.

[jhenderson]

I've got no specific comments on the source code, just one small nit in the test. I'll leave @dblaikie to continue the rest of the review.

llvm/test/tools/llvm-symbolizer/source.ll

9

It would probably be a good idea to run the exact same check for GNU output (or llvm-addr2line), just to cover our bases. That way, if the behaviour ever diverges in the future, there's a clear test to update.

[jhenderson]

llvm/test/tools/llvm-symbolizer/source.ll

2

I think "output" is a more common term to use and easier to understand.

8

No need to repeat "source context" in this text - it's implied by the top-level comment.

9

This is simpler and more traditional for the input arguments. Also CODE is the default and not needed.

17

18

20–24

There's only one small difference between this and the LLVM style, if I'm not mistaken. You could avoid duplciation by doing something like:

; RUN: ... | FileCheck ... --check-prefixes=COMMON,LLVM;
; RUN: ... | FileCheck ... --check-prefixes=COMMON,GNU

;      COMMON:foo
;   LLVM-NEXT:/source.c:3:13
;    GNU-NEXT:/source.c:3
; COMMON-NEXT:2  : // Line 2
; COMMON-NEXT:3 >: void foo() {}
; COMMON-NEXT:4  : // Line 4

26

27

[jhenderson]

Test looks good to me. I'm leaving the source for @dblaikie.

[dblaikie]

Sure, seems OK. Thanks!

[jhenderson]

Formally: LGTM!

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

protected:
   raw_ostream &OS;
   raw_ostream &ES;
   PrinterConfig Config;
 
   void print(const DILineInfo &Info, bool Inlined);
   void printFunctionName(StringRef FunctionName, bool Inlined);
   virtual void printSimpleLocation(StringRef Filename,
                                    const DILineInfo &Info) = 0;
-  void printContext(StringRef FileName, int64_t Line);
+  void printContext(const Optional<StringRef> &Source, StringRef FileName,
+                    int64_t Line);
   void printVerbose(StringRef Filename, const DILineInfo &Info);
   virtual void printFooter() {}
 
 private:
   void printHeader(uint64_t Address);
 
 public:
   PlainPrinterBase(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

 
 void PlainPrinterBase::printHeader(uint64_t Address) {
   if (Config.PrintAddress) {
     OS << "0x";
     OS.write_hex(Address);
     StringRef Delimiter = Config.Pretty ? ": " : "\n";
     OS << Delimiter;
   }
 }
 
 // Prints source code around in the FileName the Line.
-void PlainPrinterBase::printContext(StringRef FileName, int64_t Line) {
+void PlainPrinterBase::printContext(const Optional<StringRef> &Source,
+                                    StringRef FileName, int64_t Line) {
   if (Config.SourceContextLines <= 0)
     return;
 
+  std::unique_ptr<MemoryBuffer> Buf;
+  line_iterator I;
+  if (Source) {
+    I = line_iterator(MemoryBufferRef(*Source, FileName), false);
+  } else {
     ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
         MemoryBuffer::getFile(FileName);
     if (!BufOrErr)
       return;
+    Buf = std::move(*BufOrErr);
+    I = line_iterator(*Buf, false);
+  }
 
-  std::unique_ptr<MemoryBuffer> Buf = std::move(BufOrErr.get());
   int64_t FirstLine =
       std::max(static_cast<int64_t>(1), Line - Config.SourceContextLines / 2);
-  int64_t LastLine = FirstLine + Config.SourceContextLines;
+  int64_t LastLine = FirstLine + Config.SourceContextLines - 1;
   size_t MaxLineNumberWidth = std::ceil(std::log10(LastLine));
 
-  for (line_iterator I = line_iterator(*Buf, false);
-       !I.is_at_eof() && I.line_number() <= LastLine; ++I) {
+  for (; !I.is_at_eof(); ++I) {
     int64_t L = I.line_number();
-    if (L >= FirstLine && L <= LastLine) {
-      OS << format_decimal(L, MaxLineNumberWidth);
-      if (L == Line)
-        OS << " >: ";
-      else
-        OS << "  : ";
-      OS << *I << "\n";
-    }
+    if (L < FirstLine)
+      continue;
+    if (L > LastLine)
+      break;
+    OS << *I << '\n';
   }
 }
 
 void PlainPrinterBase::printFunctionName(StringRef FunctionName, bool Inlined) {
   if (Config.PrintFunctions) {
     if (FunctionName == DILineInfo::BadString)
       FunctionName = DILineInfo::Addr2LineBadString;
     StringRef Delimiter = Config.Pretty ? " at " : "\n";
     StringRef Prefix = (Config.Pretty && Inlined) ? " (inlined by) " : "";
     OS << Prefix << FunctionName << Delimiter;
   }
 }
 
 void LLVMPrinter::printSimpleLocation(StringRef Filename,
                                       const DILineInfo &Info) {
   OS << Filename << ':' << Info.Line << ':' << Info.Column << '\n';
-  printContext(Filename, Info.Line);
+  printContext(Info.Source, Filename, Info.Line);
 }
 
 void GNUPrinter::printSimpleLocation(StringRef Filename,
                                      const DILineInfo &Info) {
   OS << Filename << ':' << Info.Line;
   if (Info.Discriminator)
     OS << " (discriminator " << Info.Discriminator << ')';
   OS << '\n';
-  printContext(Filename, Info.Line);
+  printContext(Info.Source, Filename, Info.Line);
 }
 
 void PlainPrinterBase::printVerbose(StringRef Filename,
                                     const DILineInfo &Info) {
   OS << "  Filename: " << Filename << '\n';
   if (Info.StartLine) {
     OS << "  Function start filename: " << Info.StartFileName << '\n';
     OS << "  Function start line: " << Info.StartLine << '\n';
void JSONPrinter::print(const Request &Request, const DILineInfo &Info) {
   InliningInfo.addFrame(Info);
   print(Request, InliningInfo);
 }
 
 void JSONPrinter::print(const Request &Request, const DIInliningInfo &Info) {
   json::Array Array;
   for (uint32_t I = 0, N = Info.getNumberOfFrames(); I < N; ++I) {
     const DILineInfo &LineInfo = Info.getFrame(I);
-    Array.push_back(json::Object(
+    json::Object Object(
         {{"FunctionName", LineInfo.FunctionName != DILineInfo::BadString
                               ? LineInfo.FunctionName
                               : ""},
          {"StartFileName", LineInfo.StartFileName != DILineInfo::BadString
                                ? LineInfo.StartFileName
                                : ""},
          {"StartLine", LineInfo.StartLine},
          {"FileName",
           LineInfo.FileName != DILineInfo::BadString ? LineInfo.FileName : ""},
          {"Line", LineInfo.Line},
          {"Column", LineInfo.Column},
-         {"Discriminator", LineInfo.Discriminator}}));
+         {"Discriminator", LineInfo.Discriminator}});
+    if (LineInfo.Source && Config.SourceContextLines > 0) {
+      int64_t FirstLine =
+          std::max(static_cast<int64_t>(1),
+                   int64_t(LineInfo.Line) - Config.SourceContextLines / 2);
+      int64_t LastLine = FirstLine + Config.SourceContextLines - 1;
+      size_t FirstLinePos = StringRef::npos, Pos = 0;
+      for (int64_t Line = 1; Line <= LastLine; ++Line, ++Pos) {
+        if (Line == FirstLine)
+          FirstLinePos = Pos;
+        Pos = LineInfo.Source->find('\n', Pos);
+        if (Pos == StringRef::npos)
+          break;
+      }
+      if (FirstLinePos != StringRef::npos)
+        Object["Source"] = LineInfo.Source
+                               ->substr(FirstLinePos, (Pos == StringRef::npos)
+                                                          ? StringRef::npos
+                                                          : Pos - FirstLinePos)
+                               .str();
+    }
+    Array.push_back(std::move(Object));
   }
   json::Object Json = toJSON(Request);
   Json["Symbol"] = std::move(Array);
   if (ObjectList)
     ObjectList->push_back(std::move(Json));
   else
     printJSON(std::move(Json));
 }
 

llvm/test/tools/llvm-symbolizer/source.ll

+;; This test checks printout of the DWARF embedded source.
+
+; REQUIRES: x86-registered-target
+
+; RUN: llc -filetype=obj -o %t.o %s 
+
+;; Check the source context plain printout.
+; RUN: llvm-symbolizer --print-source-context-lines=3 "CODE %t.o 0" | \
+; RUN:   FileCheck %s --strict-whitespace --match-full-lines --implicit-check-not={{.}}
+;      CHECK:foo
+; CHECK-NEXT:/source.c:3:13
+; CHECK-NEXT:2  : // Line 2
+; CHECK-NEXT:3 >: void foo() {}
+; CHECK-NEXT:4  : // Line 4
+
+;; Check the source context printout to JSON.
+; RUN: llvm-symbolizer --print-source-context-lines=3 "CODE %t.o 0" --output-style=JSON | \
+; RUN:   FileCheck %s --check-prefix=JSON --strict-whitespace --match-full-lines --implicit-check-not={{.}}
+; JSON:[{"Address":"0x0","ModuleName":"{{.*}}.o","Symbol":[{"Column":13,"Discriminator":0,"FileName":"/source.c","FunctionName":"foo","Line":3,"Source":"// Line 2\nvoid foo() {}\n// Line 4\n","StartFileName":"/source.c","StartLine":3}]}]
+
+;; Generated from the following source:
+;; // Line 1
+;; // Line 2
+;; void foo() {}
+;; // Line 4
+;; // Line 5
+;; clang --target=x86_64-pc-linux -gdwarf-5 -gembed-source -g -emit-llvm -S source.c -o source.ll
+
+source_filename = "source.c"
+target triple = "x86_64-pc-linux"
+
+define dso_local void @foo() #0 !dbg !7 {
+entry:
+  ret void, !dbg !10
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "source.c", directory: "/", source: "// Line 1\0A// Line 2\0Avoid foo() {}\0A// Line 4\0A// Line 5\0A")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 5}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 12.0.0"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !DILocation(line: 3, column: 13, scope: !7)