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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


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


================
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.
----------------
ruiu wrote:
> s/, if/. If/
Done.


================
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();
----------------
ruiu wrote:
> 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.
This patch changed messages from

```
error: {{.*}}2.o:(.text+0x1): undefined symbol 'undef'
```
to
```
error: {{.*}}2.o: undefined symbol 'undef'
note: location is .text+0x1
```

and

```
error: undef.s:(.text+0x1): undefined symbol 'foo'
```

to
```
error: {{.*}}.o: undefined symbol 'foo'
note: location is undef.s(.text+0x1)
```

In that place I did not enclose " .text+0x1" with "()" when source file name is unknown intentionally,
Idea of expression enclosed is to extent primary location information, like:
primary info(secondary info). 


After addressing your comment output is:
```
note: location is (.text+0x1)
```
In this case we have no source file name, so ".text + 0x1" becomes primary information and IMO does not need
enclosing in "()".

We can change or not change it separatelly, above just an explanation of why I did that initially :)


================
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
----------------
ruiu wrote:
> 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.
Done.


https://reviews.llvm.org/D27900