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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking good, a few minor comments.



================
Comment at: ELF/Error.cpp:58-59
+  *ErrorOS << Msg << "\n";
+  if (Notes.empty())
+    return;
+  for (const std::string &Note : Notes) {
----------------
You don't need this piece of code.


================
Comment at: ELF/InputSection.cpp:212
 
-// Returns a source location string. Used to construct an error message.
+// Find a function symbol that encloses a given location, if there's no symbol,
+// returns the offset in the section.
----------------
s/, if/. If/


================
Comment at: ELF/InputSection.cpp:234-238
+  // source file name. If we have it, we use it in the output.
+  std::string FuncName = getFunctionName(this, Offset);
+  if (File->SourceFile.empty())
+    return FuncName;
+  return (File->SourceFile + "(" + FuncName + ")").str();
----------------
Looks like you are changing the output format. Previously, `function foo` was always be enclosed with `()`, but now it can be returned as a bare word. Please keep the existing message style. If you need to change that, please do that in another patch.


================
Comment at: ELF/Target.cpp:94
+
+  std::pair<std::string, std::string> Msg = getErrorLocation(Loc);
+  error(Msg.first + ": relocation " + toString(Type) + " out of range",
----------------
 


================
Comment at: test/ELF/aarch64-fpic-abs16.s:4
 // RUN: not ld.lld -shared %t.o -o %t.so 2>&1 | FileCheck %s
-// CHECK: {{.*}}:(.data+0x0): relocation R_AARCH64_ABS16 cannot be used against shared object; recompile with -fPIC.
-
+// CHECK: {{.*}}: relocation R_AARCH64_ABS16 cannot be used against shared object; recompile with -fPIC.
 .data
----------------
This and in other tests, you removed source location and didn't add a check for the new `note` output. You want to check them.


https://reviews.llvm.org/D27900