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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 22 03:44:47 PST 2016


grimar 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()));
----------------
silvas wrote:
> ruiu wrote:
> > grimar wrote:
> > > silvas wrote:
> > > > 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).
> > > > 
> > > Good point.
> > > 
> > > I would simplify diagnose() just to take std::vector<const Twine &> then. I believe we can always assume first Twine is a error and next are notes. There probably should not be 2 different errors for single place anyways, so I would not expect to have troubles because of reordering output messages here with such approach.
> > > 
> > > I can prepare such patch if this one be landed, I think it worth to do that after we'll have notes feature.
> > I think I don't like that interface because it seems much more involved than the current dead-simple warn/error/fatal functions. If you define diagnose(), we should remove warn/error/fatal because they are now duplicates, but I think we don't want to write something like `diagnose({ERROR, "cannot open file"})`. It seems unnecessarily complicated than `error("cannot open file")`.
> > 
> > What we want to do here is essentially printing out multi-line error message, right? Why don't you pass multiple lines as one string? Error messages can contain newlines.
> > If you define diagnose(), we should remove warn/error/fatal because they are now duplicates, but I think we don't want to write something like diagnose({ERROR, "cannot open file"}). It seems unnecessarily complicated than error("cannot open file").
> 
> Adding helpers to simplify / clarify common cases is a well-established software-engineering practice. For example, we have two overloads of `error`, one which just calls into the other.
> 
> >What we want to do here is essentially printing out multi-line error message, right? Why don't you pass multiple lines as one string? Error messages can contain newlines.
> 
> The code that prints the errors/notes needs to know where an error/note starts in order to print the colored "error:" or "note:" part of the diagnostic.
> Adding helpers to simplify / clarify common cases is a well-established software-engineering practice.

I agree here and think that first version of that patch was much better looking because used helpers.


https://reviews.llvm.org/D27900





More information about the llvm-commits mailing list