[PATCH] D27900: [ELF] - Keep the source file/line location information separate from the object file location information.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I quite agree with the comments by @silvas. I was trying to evaluate LLD's behaviour over the past few days, and regular saw errors with source information for objects that were never built on my machine, and so were completely meaningless. In this particular instance, it was because I was using the wrong version of an input file, but I couldn't see which file I was actually using, because I got a meaningless path in the output rather than the path to my object file.

In my opinion, all errors and warnings originating from inputs should have the form: 'error: C:\an\absolute\path\foo.o: some error' with additional information about source files in the notes that follow, which if I've understood correctly is what this patch has done, although at a glance "ELF/lto/combined-lto-object-name.ll" looks like it's the wrong way around (but I think that's just because of use of LTO...).

One comment about the code that I haven't put inline because the inline comments in Phabricator look like they may have moved around a bit, so I'm not sure where to put it - I agree that there is an awful lot of duplication in the error calling in Target.cpp, and I personally feel like a helper function would be completely appropriate in this instance. By my count there are 12 different calls to getErrorLocation followed immediately by a call to error(), with the only difference between them being the exact message in the error.



================
Comment at: ELF/InputSection.h:143
 
-  // Returns a source location string. Used to construct an error message.
-  std::string getLocation(uintX_t Offset);
+  // Returns a detailed information about source location. Used to construct an
+  // error message.
----------------
Nit: Not sure if the comment needs to change, but it should start "Returns detailed information", without the "a"


================
Comment at: test/ELF/libsearch.s:25
 // RUN:   | FileCheck --check-prefix=UNDEFINED %s
-// UNDEFINED: error: {{.*}}:(.bar+0x0): undefined symbol '_bar'
+// UNDEFINED: error: {{.*}}: undefined symbol '_bar'
 
----------------
Should this have a check for "note: location is (.bar+0x0)" or similar?


================
Comment at: test/ELF/unresolved-symbols.s:24
+# ERRUND:      error: {{.*}}: undefined symbol 'undef'
+# ERRUND-NEXT: f
 ## Also ignore all should not produce error for symbols from DSOs.
----------------
This check looks like it could easily start passing spuriously due to other error output, since it is only checking a single character. Could we at least check for "note: {{.*}} f" or equivalent for whatever the text is?


https://reviews.llvm.org/D27900