Turn local DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full-blowm DWARFDebugLine::LineTable methods.

[friss]

Summary

getFileNameForUnit() is basically a wrapper around LineTable::getFileNameByIndex().
Fold its additional functionality (adding the DWARFUnit compilation dir) into
LineTable::getFileNameByIndex().

getFileLineInfoForCompileUnit() is a wrapper around getFileNameForUnit(). As
a function to search the line information by address, it seems natural to put
it in the LineTable also.

Before this commit only the Context with its private helpers could do Linetable
lookups. This newly exposed feature will be used by the DIE dumping code to
get access to file information referenced in DIE attributes.

This commit has already been partly reviewed in D5192 and contained an
additional and a bit controversial 'realpath' call that is left out of this
patch. We can reinstate that realpath code later if it is desirable.

[samsonov]

Can you pass just a compilation dir instead of DWARFUnit* into a DWARFDebugLine methods? For now we don't need any other information from unit, and I'm not sure we will - line table should be more or less self-contained.

lib/DebugInfo/DWARFDebugLine.cpp

663 ↗

(On Diff #13706)

Looks like you can have a single SmallString<> to construct an absolute filepath.

693 ↗

(On Diff #13706)

It's not clear now why CU must be nonzero, even though getFileNameByIndex allows U to be nullptr.

lib/DebugInfo/DWARFDebugLine.h

184 ↗

(On Diff #13706)

It's somewhat confusing to have output parameter not the last parameter in the function. See my suggestion about passing compilation dir instead of DWARFUnit* - in this case you can just pass empty string if unit is unavailable.

[friss]

lib/DebugInfo/DWARFDebugLine.cpp

663 ↗

(On Diff #13706)

You mean removing the AbsolutePath variable in the is_relative code bellow right?

693 ↗

(On Diff #13706)

This is just how the original code is written, this patch is supposed to have no functional change. I agree with you that this looks strange, and maybe this check should be extracted and moved to the current callers.

lib/DebugInfo/DWARFDebugLine.h

184 ↗

(On Diff #13706)

Agreed. I mechanically add new parameters with a default value to the end to avoid having to modify the existing callers, but here it looks strange.
I'd rather pass a raw const char * to avoid creating unnecessary string objects though.

[samsonov]

lib/DebugInfo/DWARFDebugLine.cpp

663 ↗

(On Diff #13706)

Yes.

[friss]

As per review comments:

• Remove DWARFUnit * parameters to LineTable methods. Pass just the compilation dir instead.
• Remove useless temporaries in getFileNameByIndex
• Change the order of getFileNameByIndex arguments to be more natural

With above changes, the name getFileLineInfoForCompilationUnit didn't make any sense anymore (no more CU argument). Rename the function to getFileLineInfoForAddress instead.

[samsonov]

lib/DebugInfo/DWARFDebugLine.cpp

11 ↗

(On Diff #13754)

And you don't need these headers.

13 ↗

(On Diff #13754)

and this config.h file.

668 ↗

(On Diff #13754)

You don't need this extra variable.

680 ↗

(On Diff #13754)

Just call sys::part::append(FilePath, CompDir) here.

699 ↗

(On Diff #13754)

you may use "const auto& " here.

lib/DebugInfo/DWARFDebugLine.h

23 ↗

(On Diff #13754)

Now you don't need these forward declarations.

177 ↗

(On Diff #13754)

Looks like we can make this a private method now (in a separate change, after this one goes in).

188 ↗

(On Diff #13754)

Add a brief comment for this function.

[friss]

• remove now useless includes and forward declarations.
• add comment to new member function
• code changes as per review comments

[samsonov]

LGTM

[friss]

Closed by commit rL218125 (authored by @friss).

llvm/trunk/lib/DebugInfo/DWARFContext.cpp

 
 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 getFileNameForUnit(DWARFCompileUnit *U,
-                               const DWARFLineTable *LineTable,
-                               uint64_t FileIndex, FileLineInfoKind Kind,
-                               std::string &FileName) {
-  if (!U || !LineTable || Kind == FileLineInfoKind::None ||
-      !LineTable->getFileNameByIndex(FileIndex, Kind, FileName))
-    return false;
-  if (Kind == FileLineInfoKind::AbsoluteFilePath &&
-      sys::path::is_relative(FileName)) {
-    // We may still need to append compilation directory of compile unit.
-    SmallString<16> AbsolutePath;
-    if (const char *CompilationDir = U->getCompilationDir()) {
-      sys::path::append(AbsolutePath, CompilationDir);
-    }
-    sys::path::append(AbsolutePath, FileName);
-    FileName = AbsolutePath.str();
-  }
-  return true;
-}
-
-static bool getFileLineInfoForCompileUnit(DWARFCompileUnit *CU,
-                                          const DWARFLineTable *LineTable,
-                                          uint64_t Address,
-                                          FileLineInfoKind Kind,
-                                          DILineInfo &Result) {
-  if (!CU || !LineTable)
-    return false;
-  // Get the index of row we're looking for in the line table.
-  uint32_t RowIndex = LineTable->lookupAddress(Address);
-  if (RowIndex == -1U)
-    return false;
-  // Take file number and line/column from the row.
-  const DWARFDebugLine::Row &Row = LineTable->Rows[RowIndex];
-  if (!getFileNameForUnit(CU, LineTable, Row.File, Kind,
-                          Result.FileName))
-    return false;
-  Result.Line = Row.Line;
-  Result.Column = Row.Column;
-  return true;
-}
-
 static bool getFunctionNameForAddress(DWARFCompileUnit *CU, uint64_t Address,
                                       FunctionNameKind Kind,
                                       std::string &FunctionName) {
   if (Kind == FunctionNameKind::None)
     return false;
   // 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.
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);
   if (Spec.FLIKind != FileLineInfoKind::None) {
-    const DWARFLineTable *LineTable = getLineTableForUnit(CU);
-    getFileLineInfoForCompileUnit(CU, LineTable, Address, Spec.FLIKind, Result);
+    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;
DWARFContext::getLineInfoForAddressRange(uint64_t Address, uint64_t Size,
   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;
-    getFileNameForUnit(CU, LineTable, Row.File, Spec.FLIKind,
-                       Result.FileName);
+    LineTable->getFileNameByIndex(Row.File, CU->getCompilationDir(),
+                                  Spec.FLIKind, Result.FileName);
     Result.FunctionName = FunctionName;
     Result.Line = Row.Line;
     Result.Column = Row.Column;
     Lines.push_back(std::make_pair(Row.Address, Result));
   }
 
   return Lines;
 }
DWARFContext::getInliningInfoForAddress(uint64_t Address,
   const DWARFDebugInfoEntryInlinedChain &InlinedChain =
       CU->getInlinedChainForAddress(Address);
   if (InlinedChain.DIEs.size() == 0) {
     // If there is no DIE for address (e.g. it is in unavailable .dwo file),
     // try to at least get file/line info from symbol table.
     if (Spec.FLIKind != FileLineInfoKind::None) {
       DILineInfo Frame;
       LineTable = getLineTableForUnit(CU);
-      if (getFileLineInfoForCompileUnit(CU, LineTable, Address, Spec.FLIKind,
-                                        Frame)) {
+      if (LineTable &&
+          LineTable->getFileLineInfoForAddress(Address, CU->getCompilationDir(),
+                                               Spec.FLIKind, Frame))
         InliningInfo.addFrame(Frame);
-      }
-    }
     return InliningInfo;
   }
 
   uint32_t CallFile = 0, CallLine = 0, CallColumn = 0;
   for (uint32_t i = 0, n = InlinedChain.DIEs.size(); i != n; i++) {
     const DWARFDebugInfoEntryMinimal &FunctionDIE = InlinedChain.DIEs[i];
     DILineInfo Frame;
     // Get function name if necessary.
     if (const char *Name =
             FunctionDIE.getSubroutineName(InlinedChain.U, Spec.FNKind))
       Frame.FunctionName = Name;
     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.
-        getFileLineInfoForCompileUnit(CU, LineTable, Address, Spec.FLIKind,
-                                      Frame);
+        if (LineTable)
+          LineTable->getFileLineInfoForAddress(Address, CU->getCompilationDir(),
+                                               Spec.FLIKind, Frame);
       } else {
         // Otherwise, use call file, call line and call column from
         // previous DIE in inlined chain.
-        getFileNameForUnit(CU, LineTable, CallFile, Spec.FLIKind,
-                           Frame.FileName);
+        if (LineTable)
+          LineTable->getFileNameByIndex(CallFile, CU->getCompilationDir(),
+                                        Spec.FLIKind, Frame.FileName);
         Frame.Line = CallLine;
         Frame.Column = CallColumn;
       }
       // Get call file/line/column of a current DIE.
       if (i + 1 < n) {
         FunctionDIE.getCallerFrame(InlinedChain.U, CallFile, CallLine,
                                    CallColumn);
       }

llvm/trunk/lib/DebugInfo/DWARFDebugLine.h

struct LineTable {
     // or -1 if there is no such row.
     uint32_t lookupAddress(uint64_t address) const;
 
     bool lookupAddressRange(uint64_t address, uint64_t size,
                             std::vector<uint32_t> &result) const;
 
     // Extracts filename by its index in filename table in prologue.
     // Returns true on success.
-    bool getFileNameByIndex(uint64_t FileIndex,
+    bool getFileNameByIndex(uint64_t FileIndex, const char *CompDir,
                             DILineInfoSpecifier::FileLineInfoKind Kind,
                             std::string &Result) const;
 
+    // Fills the Result argument with the file and line information
+    // corresponding to Address. Returns true on success.
+    bool getFileLineInfoForAddress(uint64_t Address, const char *CompDir, 
+                                   DILineInfoSpecifier::FileLineInfoKind Kind,
+                                   DILineInfo &Result) const;
+
     void dump(raw_ostream &OS) const;
     void clear();
 
     /// Parse prologue and all rows.
     bool parse(DataExtractor debug_line_data, const RelocAddrMap *RMap,
                uint32_t *offset_ptr);
 
     struct Prologue Prologue;

llvm/trunk/lib/DebugInfo/DWARFDebugLine.cpp

while (seq_pos != last_seq && seq_pos->LowPC < end_addr) {
     ++seq_pos;
   }
 
   return true;
 }
 
 bool
 DWARFDebugLine::LineTable::getFileNameByIndex(uint64_t FileIndex,
+                                              const char *CompDir,
                                               FileLineInfoKind Kind,
                                               std::string &Result) const {
   if (FileIndex == 0 || FileIndex > Prologue.FileNames.size() ||
       Kind == FileLineInfoKind::None)
     return false;
   const FileNameEntry &Entry = Prologue.FileNames[FileIndex - 1];
   const char *FileName = Entry.Name;
   if (Kind != FileLineInfoKind::AbsoluteFilePath ||
       sys::path::is_absolute(FileName)) {
     Result = FileName;
     return true;
   }
+
   SmallString<16> FilePath;
   uint64_t IncludeDirIndex = Entry.DirIdx;
+  const char *IncludeDir = "";
   // Be defensive about the contents of Entry.
   if (IncludeDirIndex > 0 &&
-      IncludeDirIndex <= Prologue.IncludeDirectories.size()) {
-    const char *IncludeDir = Prologue.IncludeDirectories[IncludeDirIndex - 1];
-    sys::path::append(FilePath, IncludeDir);
-  }
-  sys::path::append(FilePath, FileName);
+      IncludeDirIndex <= Prologue.IncludeDirectories.size())
+    IncludeDir = Prologue.IncludeDirectories[IncludeDirIndex - 1];
+
+  // We may still need to append compilation directory of compile unit.
+  // We know that FileName is not absolute, the only way to have an
+  // absolute path at this point would be if IncludeDir is absolute.
+  if (CompDir && Kind == FileLineInfoKind::AbsoluteFilePath &&
+      sys::path::is_relative(IncludeDir))
+    sys::path::append(FilePath, CompDir);
+
+  // sys::path::append skips empty strings.
+  sys::path::append(FilePath, IncludeDir, FileName);
   Result = FilePath.str();
   return true;
 }
+
+bool
+DWARFDebugLine::LineTable::getFileLineInfoForAddress(uint64_t Address,
+                                                     const char *CompDir,
+                                                     FileLineInfoKind Kind,
+                                                     DILineInfo &Result) const {
+  // Get the index of row we're looking for in the line table.
+  uint32_t RowIndex = lookupAddress(Address);
+  if (RowIndex == -1U)
+    return false;
+  // Take file number and line/column from the row.
+  const auto &Row = Rows[RowIndex];
+  if (!getFileNameByIndex(Row.File, CompDir, Kind, Result.FileName))
+    return false;
+  Result.Line = Row.Line;
+  Result.Column = Row.Column;
+  return true;
+}