[PATCH] D38721: [ELF] - Teach LLD to report line numbers for data symbols.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputFiles.cpp:123
+
+  // The offset to CU is 0.
+  const DWARFDebugLine::LineTable *LT = DwarfLine->getLineTable(0);
----------------
grimar wrote:
> ruiu wrote:
> > I don't get the meaning of this comment.
> It is the same as:
> https://github.com/llvm-mirror/lld/blob/master/ELF/InputFiles.cpp#L77
> 
> We always have one compilation unit here, so offset is always 0. 
> I supposed it is fine because we have equal comment here:
> https://github.com/llvm-mirror/lld/blob/master/ELF/InputFiles.cpp#L90
> 
> I extended this comment.
The have -> There is always only one


================
Comment at: ELF/InputFiles.h:204
   void initializeSymbols();
-  void initializeDwarfLine();
+  void initializeDwarfData();
   InputSectionBase *getRelocTarget(const Elf_Shdr &Sec);
----------------
Omit "Data" as DWARF is always data.


================
Comment at: ELF/InputFiles.h:220
   std::unique_ptr<llvm::DWARFDebugLine> DwarfLine;
+  llvm::DenseMap<StringRef, std::pair<unsigned, unsigned>> DataObjectsLoc;
   llvm::once_flag InitDwarfLine;
----------------
VariableLoc


================
Comment at: ELF/InputSection.cpp:264-265
 
-// Returns a source location string. This function is intended to be
-// used for constructing an error message. The returned message looks
-// like this:
+// Function concatenates Path and Line provided. It is used for
+// constructing error messages containing source location.
+static std::string createFileLineMsg(StringRef Path, unsigned Line) {
----------------
Function -> "A function" or "this function"

But maybe it is better to omit the subject.

// Concatenates arguments to construct a string representing an error location.


================
Comment at: ELF/InputSection.cpp:289
+  // We want to get location string here. For functions we take information from
+  // .debug_line which contains association between location in source files and
+  // machine instuctions. But for data objects we have to scan DIEs in .debug_info
----------------
association between location in sourcef iles and machine instructions

->

table from offsets to source locations


================
Comment at: ELF/InputSection.cpp:290-291
+  // .debug_line which contains association between location in source files and
+  // machine instuctions. But for data objects we have to scan DIEs in .debug_info
+  // which describes variables and their locations.
+  if (Sym.Type == STT_OBJECT) {
----------------
For variables, we have to use .debug_info instead.


================
Comment at: ELF/InputSection.cpp:292
+  // which describes variables and their locations.
+  if (Sym.Type == STT_OBJECT) {
+    if (Optional<std::pair<std::string, unsigned>> FileLine =
----------------
Are you sure that functions would never have type STT_OBJECT?


https://reviews.llvm.org/D38721