Get function start line number from DWARF info

[sque]

Summary

DWARF info contains info about the line number at which a function starts (DW_AT_decl_line).

This patch creates a function to look up the start line number for a function, and returns it in
DILineInfo when looking up debug info for a particular address.

[aprantl]

What's use-case for this?

Also, could you add a test-case?

[sque]

We have some changes within Google code that depend on this. echristo has already reviewed the internal changes.

I'm still not familiar with the way tests work for this kind of code. Can you point me to a prior commit to lib/DebugInfo/DWARF that has a test case, so I can use it as an example?

[aprantl]

If the change is observable from llvm-dwarfdump, you can add a textual test using FileCheck. Otherwise (and it looks like this is the case here) you can add a unit test to unittests/unittests/DebugInfo/DWARF/*.

Process side note: Having "internal changes that depend on it" is typically not a good enough reason to add code to LLVM. Every line of code added to LLVM adds a maintenance burden for the community, so it is typically better to argue why a change benefits LLVM or other downstream users when submitting a patch for review :-)

[aprantl]

lib/DebugInfo/DWARF/DWARFContext.cpp

58

Why do you need to demangle the name here?

72

Shouldn't this be using accelerator tables if available?

[probinson]

This seems like it's doing way more work than necessary. AFAICT each of the callers is already having to figure out the InlinedChain, which hands you a DIE for the function. It might be the DIE you actually want, or it might have a DW_AT_specification or DW_AT_abstract_origin that is a reference to the DIE you actually want. I can't see any reason to extract a name and then try to do a (possibly ambiguous) name search through the entire debug-info section.
Am I missing something?

[echristo]

I'm not sure why you need this as part of the line number lookup. Can you explain a bit more how the code that uses this works and why you need this here?

Thanks!

lib/DebugInfo/DWARF/DWARFContext.cpp

58

Agreed. Also, this needs to be llvm style.

72

Possibly, but let's add those on after...

[sque]

Simplified code to get start line

[sque]

PTAL. I still need to add a unit test.

[aprantl]

lib/DebugInfo/DWARF/DWARFContext.cpp

442

We're trying to get rid of all DWARF APIs that use special failure values right now. Could this return an Optional<uint32_t> instead?

[clayborg]

If "StartLine" is actually DW_AT_decl_line, then I would suggest calling it "DeclLine". See other inlined comments.

include/llvm/DebugInfo/DIContext.h

37

If this actually is always the value of DW_AT_decl_line, why not name it accordingly? "DeclLine" maybe instead of "StartLine"? StartLine seems confusing.

lib/DebugInfo/DWARF/DWARFContext.cpp

442

This should be added to DWARFDie as a member function:

Optional<uint32_t> DWARFDie::getDeclLine() const;

Other clients will want the DW_AT_decl_line and we shouldn't write the code more than once.

[sque]

I've added a unit test: https://reviews.llvm.org/D28428

I'll wait until that patch is approved before updating this one, which will modify that patch.

include/llvm/DebugInfo/DIContext.h

37

It depends on what this field is called in other formats. I only know that it is called "decl_line" in DWARF. Maybe this should be under the "DWARF-specific" section if we call it "DeclLine"

lib/DebugInfo/DWARF/DWARFContext.cpp

442

Good idea.

[sque]

Add function to get decl line from DWARFDie

[clayborg]

lib/DebugInfo/DWARF/DWARFDie.cpp

347 ↗

(On Diff #83751)

Remove the -1U here, it will return an optional for you. You can just do:

if (auto DeclLine = getAttributeValueAsUnsignedConstant(DW_AT_decl_line))
  return DeclLine;

353–356 ↗

(On Diff #83751)

Recurse in case there is more than one specification or abstract origin:

if (auto DeclLine = SpecDie.getDeclLine())
  return DeclLine;

361–364 ↗

(On Diff #83751)

Ditto

[sque]

Recursively get specification or abstract origin; omit use of failure value

[clayborg]

DWARFDie stuff looks good. I will let others comment on the rest.

[sque]

Reorganize get function name logic in getFunctionNameAndStartLineForAddress

[sque]

Reorganize function name logic

[sque]

PTAL.

[dblaikie]

lib/DebugInfo/DWARF/DWARFDie.cpp

347–349 ↗

(On Diff #83865)

Roll the variable declaration into the use:

if (auto DeclLine = getAttributeAs...)
  return DeclLine;

352–354 ↗

(On Diff #83865)

And here

357–359 ↗

(On Diff #83865)

Similarly here:

if (auto AbsDie = ...)

if (auto DeclLine = ...)
  return DeclLine;

[dblaikie]

Also - what's the use case for this? (& test coverage would be good/necessary)

[sque]

Updated with dblaikie's suggestions (roll variable declaration into use)

[sque]

In D27962#647456, @dblaikie wrote:

Also - what's the use case for this? (& test coverage would be good/necessary)

Use case: Internal use case, will email you about it.

Unit test: No unit test exists for DWARFContext, but I added one here, awaiting review: https://reviews.llvm.org/D28428

(Both are already mentioned earlier in this thread)

[sque]

Print start line in DIPrinter, used by LLVM Symbolizer

[sque]

In D27962#649064, @sque wrote:

Print start line in DIPrinter, used by LLVM Symbolizer

Example output:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

[echristo]

In D27962#649067, @sque wrote:

In D27962#649064, @sque wrote:

Print start line in DIPrinter, used by LLVM Symbolizer

Example output:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

Hrm. But it already had the line above?

[sque]

In D27962#649079, @echristo wrote:

0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

Hrm. But it already had the line above?

Bad example, that was the first line of the function. How about:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f2d
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:90:0
Start line: 89

0x406f30
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:90:0
Start line: 89

0x406f40
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:91:0
Start line: 89

0x406f50
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:93:0
Start line: 89

[sque]

Rebased to latest revision; Added regression test coverage

[dblaikie]

Looks good - a few minor improvements/tidyups that'd be good.

lib/DebugInfo/DWARF/DWARFContext.cpp

589–590

Roll the declaration into the if condition:

if (auto DeclLineResult = FunctionDIE.getDeclLine())

(up to you if you use auto - whatever you think's more readable)

lib/DebugInfo/DWARF/DWARFDie.cpp

295–307 ↗

(On Diff #86695)

I think this whole function can now be implemented with this:

return toUnsigned(findRecursively(DW_AT_line));

test/tools/llvm-symbolizer/sym-verbose.test

28 ↗

(On Diff #86695)

Might be good to describe this as "Function start line" to be more clear (& maybe put it above "Line" - so that line/col/discrim are together, since they're closely related.

[sque]

Responded to dblaikie's comments

[sque]

Move printing of start line to before line/column/discrim

include/llvm/DebugInfo/DIContext.h

 class raw_ostream;
 
 /// DILineInfo - a format-neutral container for source line information.
 struct DILineInfo {
   std::string FileName;
   std::string FunctionName;
   uint32_t Line;
   uint32_t Column;
+  uint32_t StartLine;
 
   // DWARF-specific.
   uint32_t Discriminator;
 
   DILineInfo()
       : FileName("<invalid>"), FunctionName("<invalid>"), Line(0), Column(0),
-        Discriminator(0) {}
+        StartLine(0), Discriminator(0) {}
 
   bool operator==(const DILineInfo &RHS) const {
     return Line == RHS.Line && Column == RHS.Column &&
-           FileName == RHS.FileName && FunctionName == RHS.FunctionName;
+           FileName == RHS.FileName && FunctionName == RHS.FunctionName &&
+           StartLine == RHS.StartLine && Discriminator == RHS.Discriminator;
   }
   bool operator!=(const DILineInfo &RHS) const {
     return !(*this == RHS);
   }
   bool operator<(const DILineInfo &RHS) const {
-    return std::tie(FileName, FunctionName, Line, Column) <
-           std::tie(RHS.FileName, RHS.FunctionName, RHS.Line, RHS.Column);
+    return std::tie(FileName, FunctionName, Line, Column, StartLine,
+                    Discriminator) <
+           std::tie(RHS.FileName, RHS.FunctionName, RHS.Line, RHS.Column,
+                    RHS.StartLine, RHS.Discriminator);
   }
 };
 
 typedef SmallVector<std::pair<uint64_t, DILineInfo>, 16> DILineInfoTable;
 
 /// DIInliningInfo - a format-neutral container for inlined code description.
 class DIInliningInfo {
   SmallVector<DILineInfo, 4> Frames;

lib/DebugInfo/DWARF/DWARFContext.cpp

void DWARFContext::dump(raw_ostream &OS, DIDumpType DumpType, bool DumpEH,
   if (DumpType == DIDT_All || DumpType == DIDT_Abbrev) {
     OS << ".debug_abbrev contents:\n";
     getDebugAbbrev()->dump(OS);
   }
 
   if (DumpType == DIDT_All || DumpType == DIDT_AbbrevDwo)
     if (const DWARFDebugAbbrev *D = getDebugAbbrevDWO()) {
       OS << "\n.debug_abbrev.dwo contents:\n";
       D->dump(OS);
     }
 
   if (DumpType == DIDT_All || DumpType == DIDT_Info) {
     OS << "\n.debug_info contents:\n";
     for (const auto &CU : compile_units())
       CU->dump(OS);
   }
 
   if ((DumpType == DIDT_All || DumpType == DIDT_InfoDwo) &&
       getNumDWOCompileUnits()) {
     OS << "\n.debug_info.dwo contents:\n";
     for (const auto &DWOCU : dwo_compile_units())
       DWOCU->dump(OS);
   }
 
   if ((DumpType == DIDT_All || DumpType == DIDT_Types) && getNumTypeUnits()) {
     OS << "\n.debug_types contents:\n";
     for (const auto &TUS : type_unit_sections())
       for (const auto &TU : TUS)
         TU->dump(OS, SummarizeTypes);
   }
 

 
 DWARFCompileUnit *DWARFContext::getCompileUnitForAddress(uint64_t Address) {
   // First, get the offset of the compile unit.
   uint32_t CUOffset = getDebugAranges()->findAddress(Address);
   // Retrieve the compile unit.
   return getCompileUnitForOffset(CUOffset);
 }
 
-static bool getFunctionNameForAddress(DWARFCompileUnit *CU, uint64_t Address,
+static uint32_t getStartLineFromDie(const DWARFDie &DIE, uint32_t FailValue) {
+  uint32_t StartLine =
+      DIE.getAttributeValueAsUnsignedConstant(DW_AT_decl_line, FailValue);
+  if (StartLine == FailValue) {
+    // If it was not found, try to get it from specification DIE.
+    DWARFDie SpecDIE =
+        DIE.getAttributeValueAsReferencedDie(DW_AT_specification);
+    if (SpecDIE) {
+      StartLine = SpecDIE.getAttributeValueAsUnsignedConstant(DW_AT_decl_line,
+                                                              FailValue);
+    }
+  }
+  if (StartLine == FailValue) {
+    // If it was not found, try to get it from abstract origin DIE.
+    DWARFDie AbsDIE =
+        DIE.getAttributeValueAsReferencedDie(DW_AT_abstract_origin);
+    if (AbsDIE)
+      StartLine = AbsDIE.getAttributeValueAsUnsignedConstant(DW_AT_decl_line,
+                                                             FailValue);
+  }
+  return StartLine;
+}
+
+static bool getFunctionNameAndStartLineForAddress(DWARFCompileUnit *CU,
+                                                  uint64_t Address,
                                                   FunctionNameKind Kind,
-                                      std::string &FunctionName) {
-  if (Kind == FunctionNameKind::None)
-    return false;
+                                                  std::string &FunctionName,
+                                                  uint32_t &StartLine) {
   // The address may correspond to instruction in some inlined function,
   // so we have to build the chain of inlined functions and take the
-  // name of the topmost function in it.SmallVectorImpl<DWARFDie> &InlinedChain
+  // name of the topmost function in it.
   SmallVector<DWARFDie, 4> InlinedChain;
   CU->getInlinedChainForAddress(Address, InlinedChain);
-  if (InlinedChain.size() == 0)
+  if (InlinedChain.empty())
     return false;
-  if (const char *Name = InlinedChain[0].getSubroutineName(Kind)) {
+
+  const DWARFDie &DIE = InlinedChain[0];
+  bool FoundResult = false;
+  const char *Name = DIE.getSubroutineName(Kind);
+  if (Kind != FunctionNameKind::None && Name) {
     FunctionName = Name;
-    return true;
+    FoundResult = true;
   }
-  return false;
+  StartLine = getStartLineFromDie(DIE, -1U);
+  if (StartLine != -1U)
+    FoundResult = true;
+
+  return FoundResult;
 }
 
 DILineInfo DWARFContext::getLineInfoForAddress(uint64_t Address,
                                                DILineInfoSpecifier Spec) {
   DILineInfo Result;
 
   DWARFCompileUnit *CU = getCompileUnitForAddress(Address);
   if (!CU)
     return Result;
-  getFunctionNameForAddress(CU, Address, Spec.FNKind, Result.FunctionName);
+  getFunctionNameAndStartLineForAddress(CU, Address, Spec.FNKind,
+                                        Result.FunctionName,
+                                        Result.StartLine);
   if (Spec.FLIKind != FileLineInfoKind::None) {
     if (const DWARFLineTable *LineTable = getLineTableForUnit(CU))
       LineTable->getFileLineInfoForAddress(Address, CU->getCompilationDir(),
                                            Spec.FLIKind, Result);
   }
   return Result;
 }
 
 DILineInfoTable
 DWARFContext::getLineInfoForAddressRange(uint64_t Address, uint64_t Size,
                                          DILineInfoSpecifier Spec) {
   DILineInfoTable  Lines;
   DWARFCompileUnit *CU = getCompileUnitForAddress(Address);
   if (!CU)
     return Lines;
 
   std::string FunctionName = "<invalid>";
-  getFunctionNameForAddress(CU, Address, Spec.FNKind, FunctionName);
+  uint32_t StartLine = 0;
+  getFunctionNameAndStartLineForAddress(CU, Address, Spec.FNKind, FunctionName,
+                                        StartLine);
 
   // If the Specifier says we don't need FileLineInfo, just
   // return the top-most function at the starting address.
   if (Spec.FLIKind == FileLineInfoKind::None) {
     DILineInfo Result;
     Result.FunctionName = FunctionName;
+    Result.StartLine = StartLine;
     Lines.push_back(std::make_pair(Address, Result));
     return Lines;
   }
 
   const DWARFLineTable *LineTable = getLineTableForUnit(CU);
 
   // Get the index of row we're looking for in the line table.
   std::vector<uint32_t> RowVector;
   if (!LineTable->lookupAddressRange(Address, Size, RowVector))
     return Lines;
 
   for (uint32_t RowIndex : RowVector) {
     // Take file number and line/column from the row.
     const DWARFDebugLine::Row &Row = LineTable->Rows[RowIndex];
     DILineInfo Result;
     LineTable->getFileNameByIndex(Row.File, CU->getCompilationDir(),
                                   Spec.FLIKind, Result.FileName);
     Result.FunctionName = FunctionName;
     Result.Line = Row.Line;
     Result.Column = Row.Column;
+    Result.StartLine = StartLine;
     Lines.push_back(std::make_pair(Row.Address, Result));
   }
 
   return Lines;
 }
 
 DIInliningInfo
 DWARFContext::getInliningInfoForAddress(uint64_t Address,
DWARFContext::getInliningInfoForAddress(uint64_t Address,
 
   uint32_t CallFile = 0, CallLine = 0, CallColumn = 0;
   for (uint32_t i = 0, n = InlinedChain.size(); i != n; i++) {
     DWARFDie &FunctionDIE = InlinedChain[i];
     DILineInfo Frame;
     // Get function name if necessary.
     if (const char *Name = FunctionDIE.getSubroutineName(Spec.FNKind))
       Frame.FunctionName = Name;
+    Frame.StartLine = getStartLineFromDie(FunctionDIE, -1U);
     if (Spec.FLIKind != FileLineInfoKind::None) {
       if (i == 0) {
         // For the topmost frame, initialize the line table of this
         // compile unit and fetch file/line info from it.
         LineTable = getLineTableForUnit(CU);
         // For the topmost routine, get file/line info from line table.
         if (LineTable)
           LineTable->getFileLineInfoForAddress(Address, CU->getCompilationDir(),
                                                Spec.FLIKind, Frame);