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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Some parts of lld's source code is quite hard to understand now as an accumulated result of numerous patches. I'm trying to fix it now, so please spend a bit more time too to improve code quality so that it is understandable for future readers.



================
Comment at: ELF/InputFiles.cpp:99
+
+    unsigned File = dwarf::toUnsigned(Die.find(dwarf::DW_AT_decl_file), 0);
+    if (!LT->hasFileAtIndex(File))
----------------
Skip something ... skip other thing... and now what? This block needs comment too.


================
Comment at: ELF/InputFiles.cpp:109
+
+template <class ELFT>
+llvm::Optional<std::pair<std::string, unsigned>>
----------------
What does this do?


================
Comment at: ELF/InputFiles.cpp:110
+template <class ELFT>
+llvm::Optional<std::pair<std::string, unsigned>>
+ObjFile<ELFT>::getVariableLoc(StringRef Name) {
----------------
Remove `llvm::`



================
Comment at: ELF/InputSection.cpp:281
 
+static std::string getFileLineMsg(StringRef Path, unsigned Line) {
+  std::string Filename = path::filename(Path);
----------------
get -> create

(Unless it is an accessor, "get" is not a good prefix.)


================
Comment at: ELF/InputSection.cpp:281
 
+static std::string getFileLineMsg(StringRef Path, unsigned Line) {
+  std::string Filename = path::filename(Path);
----------------
ruiu wrote:
> get -> create
> 
> (Unless it is an accessor, "get" is not a good prefix.)
It needs comment.


================
Comment at: ELF/InputSection.cpp:309
+  if (Sym.Type == STT_OBJECT) {
+    if (llvm::Optional<std::pair<std::string, unsigned>> FileLine =
+            File->getVariableLoc(Sym.getName()))
----------------
Remove `llvm::`.


================
Comment at: test/ELF/Inputs/conflict-debug-variable.s:1
+# Used reduced output from following code and gcc 7.1.0
+# to produce this input file:
----------------
I don't think we want to have this machine-generated assembly in the test directory. It is hard to verify/edit. Can you do this with llvm-as?


================
Comment at: test/ELF/conflict.s:45
 
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN:   %p/Inputs/conflict-debug-variable.s -o %t-dbg2.o
----------------
Please create a new test file.


https://reviews.llvm.org/D38721