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

[Sean Silva via Phabricator via llvm-commits]

silvas added inline comments.


================
Comment at: ELF/SymbolTable.cpp:373
+        toString(*Existing) + "'");
+  note("definition found in " + D->Section->getLocation(D->Value));
+  note("previous definition was here: " + toString(D->Section->getFile()));
----------------
If we are emitting an error that needs to be followed by a `note`, they should follow each other. So they both need to be printed while holding the diagnostic mutex. We cannot release the mutex in between or else the notes could get interleaved with other diagnostics.

This issue I think existed before this patch, so we can probably not worry about it during this patch. But it can lead to a poor user experience (avoiding interleaved diagnostics is a main benefit of Ninja over make, so users seem to care about this feature).

I don't think this particular code path is called in a multithreaded context, but it is something to think about.

It is easy to end up with a very complex design for this, but the simplest one I can think of is just to have a single `diagnose` function for recoverable errors. Then instead of:

```
error("foo");
note("bar");
```

We instead have:
```
diagnose({
  Error("foo"),
  Note("bar")
});
```

or maybe
```
diagnose({
  {ERROR, "foo"},
  {NOTE, "bar"}
});
```

(`diagnose` would take a `std::vector<std::pair<ErrorKind, const Twine &>>` or something like that.

For simple cases, we would still use the `error` function (it can just forward to `diagnose`).

Alternatively, the `error` function can have a second argument which is a list of notes to emit with the error -- this would work unless we ever want to emit a note by itself (I don't think we ever want to do that though).



https://reviews.llvm.org/D27900