[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:90
+  for (const auto &Entry : CU->dies()) {
+    DWARFDie Die = {CU, &Entry};
+    // Skip all tags that are not variables.
----------------
Die(CU, &Entry)


================
Comment at: ELF/InputFiles.cpp:120
+Optional<std::pair<std::string, unsigned>>
+ObjFile<ELFT>::getDataLoc(StringRef Name) {
+  llvm::call_once(InitDwarfLine, [this]() { initializeDwarfData(); });
----------------
Data doesn't feel quite right -- for example, an unnamed region of a section contains data, but that wasn't something you are thinking about when you wrote this function. Variable is better.


================
Comment at: ELF/InputFiles.cpp:123
+
+  // The offset to CU is 0.
+  const DWARFDebugLine::LineTable *LT = DwarfLine->getLineTable(0);
----------------
I don't get the meaning of this comment.


================
Comment at: ELF/InputSection.cpp:261-265
+// This function is intended to be used for constructing an error message.
+// The returned message looks like this:
 //
 //   foo.c:42 (/home/alice/possibly/very/long/path/foo.c:42)
 //
----------------
This comment is not quite right, because it returns a string similar to the example only when you give a pathname and a line number.

Your comment gives a false impression that it always return a string consisting with a pathname and a line number. That is not true, because it is not really a property of this function but a property of a caller of this function. What this function does is just blindly concatenating strings.


================
Comment at: ELF/InputSection.cpp:274-275
+
+// Returns a source location string. It normally takes this information from
+// debug sections and used for constructing an error message. Returns an empty
+// string if there's no location information available.
----------------
What is "this", and what do you mean by "normally"?

I'd keep the original comment

  // This function is intended to be used for constructing an error message. 
  // The returned message looks like this:
  //
  //   foo.c:42 (/home/alice/possibly/very/long/path/foo.c:42)
  //
  //  Returns an empty string if there's no way to get line info.


https://reviews.llvm.org/D38721