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

[George Rimar via Phabricator via llvm-commits]

grimar 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.
----------------
ruiu wrote:
> Die(CU, &Entry)
Done.


================
Comment at: ELF/InputFiles.cpp:123
+
+  // The offset to CU is 0.
+  const DWARFDebugLine::LineTable *LT = DwarfLine->getLineTable(0);
----------------
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.


================
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)
 //
----------------
ruiu wrote:
> 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.
I see. Rewrote the comment.


================
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.
----------------
ruiu wrote:
> 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.
Restored original comment. 
I used word "normally" because when no debug information is available, it tries
to take information about location from STT_FILE symbol.
So if I would omit "normally", it would be not entirely correct.


https://reviews.llvm.org/D38721