Page MenuHomePhabricator

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

Authored by grimar on Dec 18 2016, 6:13 AM.

Details

Summary

Approach was suggested in review for D27676,
patch splits reporting object name and source file/line location.
Extended information about source name, line etc moved to "note:" output.

Example of new output:
error: {{.*}}.o: duplicate symbol 'zed'
note: definition found in conflict-debug.s:4
previous definition was here: {{.*}}.o
definition found in conflict-debug.s:4

Diff Detail

Event Timeline

grimar updated this revision to Diff 81880.Dec 18 2016, 6:13 AM
grimar retitled this revision from to [ELF] - Keep the source file/line location information separate from the object file location information..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: silvas, davide, llvm-commits and 2 others.
error: file-dbg2.o: duplicate symbol 'zed'
error: file-dbg2.a(file-dbg2.o): previous definition was here
note: definitions found in conflict-debug.s(.text+0x0) and conflict-debug.s(.text+0x0)

In general, we should assume that file names are going to be very long, so we should avoid having two of them on the same line. Can you follow the approach that the clang diagnostics use? (I pasted an example in: https://reviews.llvm.org/D27676#622869). I.e. "error: note: error: note:"

grimar updated this revision to Diff 81988.Dec 19 2016, 1:07 PM
  • Addressed comments.
grimar updated this revision to Diff 81989.Dec 19 2016, 1:09 PM
  • Uploading correct diff now :]

In general, we should assume that file names are going to be very long, so we should avoid having two of them on the same line. Can you follow the approach that the clang diagnostics use? (I pasted an example in: https://reviews.llvm.org/D27676#622869). I.e. "error: note: error: note:"

Hopefully I implemented your suggestion now. Is that what you want to see ?

ruiu edited edge metadata.Dec 20 2016, 12:06 AM

Overall, I think you designed this a bit too much. I don't want to add that many small wrapper functions for the existing error/warn/fatal functions.

ELF/Error.cpp
95

If you add hint after warn in the header, please add to the same place in .cpp.

ELF/Error.h
39–40

Remove this blank line.

ELF/InputSection.cpp
46

I don't think this is a good addition because we already have too many error() wrapper functions. Aggregating two function calls into one doesn't help us that much, and I think it actually decreases readability. Please write it in two functions.

179

This is another Ext and that should be avoided.

ELF/InputSection.h
153

Please avoid Ext suffix because it doesn't convey meaning (with that naming scheme we'll end up having something like getDataExtExt.) You need to name variables/functions based on what they are instead of whether they are new or old.

ELF/SymbolTable.cpp
373–379

Why do you need this, OldLoc and OldLocHint? You can just write in place.

377

This results in an odd message.

note: foo.o: previous definition was here

This should be

note: previous definition was here: foo.o
ELF/Target.cpp
93

Please avoid writing a new wrapper.

99

Please avoid writing a new wrapper function.

What is this function? I think we don't want to print out "fatal error encountered" at end.

grimar added inline comments.Dec 20 2016, 12:16 AM
ELF/Target.cpp
99

Target.cpp now sometimes fatal(), for example when reloc is unrecognized.
So actually problem is to call some error message + hint(). I can't just call

fatal(...).
hint(...)

because user will not see hint.

That is why I introduced this function. Though I think no need to worry about it probably.
I believe what we can do at first - is to change fatal() to errors() in a separate patch. That removes
need of such hack. I'll try to prepare a patch, not sure why we fatal() now.

grimar updated this revision to Diff 82121.Dec 20 2016, 10:48 AM
grimar edited edge metadata.
  • Addressed review comments.
grimar added inline comments.Dec 20 2016, 10:49 AM
ELF/Error.cpp
95

Done.

ELF/Error.h
39–40

Done, though that was done intentionally initially. "hint" is not a error and not a log/warning,
but I have no major preference here to keep it separate from first group too.

Also I renamed "hint" to "note". As it prints "note: ....".
Actually I am thinking about this as about "hint", that why named in that way. But consistency with clang probably matters here.

ELF/InputSection.cpp
46

I am also would not do that change. I see no problems in having one more wrapper. It incapsulates some logic and avoids code duplication.
Sometimes we have a function for single call, like "convertPathToUnix" or alike for readability and that is fine.
Actually problem that I see is that them are thown everywhere around in code but not in a local file intended for error reporting. I would suggest to make error reporting to be as much local as possible and just move that to error.cpp.

So, I did that change as well, but I do not feel it was good one.

179

Done.

ELF/InputSection.h
153

I renamed it back. But I would like to explane why I did that (actually your mail about communication issues about LLD had a good effect for me, so I probably will want sometimes to explain why I did the changes instead of just replying "Done" as I did before).

Initially during development of this patch I had two methods. They were getLocationBase (it was inlined finally) and this one, getLocationExt.
I thought about name of last one. "getLocationForOffset", "getExtendedLocation", "getLocationHint". "getLocation" just was not ideal naming for me, because it did not reveal this function used for note/hint to provide additional info uncovered by getLocationBase.
Now I do not have "getLocationBase", but that does not mean purpose of "getLocationExt" changed, it still returns something more than a regular location.

ELF/SymbolTable.cpp
373–379

Done. I feel my way was a bit more readable though.

377

Done.

ELF/Target.cpp
93

Done. But I do not understand why. New code looks a bit bulky and overduplicated after doing that change.

99

So patch for this was D27973. I replaced calls with error() instead of fatal() here assuming it be landed first.

But please look at EhReader::failOn(). I still have to call fatal() there.
I think I can prepare a patch to avoid fatal() in EhReader, what do you think ?

silvas added inline comments.Dec 20 2016, 2:26 PM
ELF/SymbolTable.cpp
375

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).

grimar added inline comments.Dec 20 2016, 11:39 PM
ELF/SymbolTable.cpp
375

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.

ruiu added inline comments.Dec 21 2016, 12:42 AM
ELF/SymbolTable.cpp
375

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.

silvas added inline comments.Dec 21 2016, 11:21 AM
ELF/SymbolTable.cpp
375

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.

grimar added inline comments.Dec 22 2016, 3:44 AM
ELF/SymbolTable.cpp
375

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.

ruiu added a comment.Jan 9 2017, 6:51 PM

How about this? You can change the type of the function from

void error(const Twine &Msg);

to

void error(const Twine &Msg, const Twine &Note = "");

Then you can eliminate note() function, and it will also solve the problem that you cannot call note() function after fatal().

grimar updated this revision to Diff 83794.Jan 10 2017, 3:51 AM
  • Addressed review comment.
In D27900#640961, @ruiu wrote:

How about this? You can change the type of the function from

void error(const Twine &Msg);

to

void error(const Twine &Msg, const Twine &Note = "");

Then you can eliminate note() function, and it will also solve the problem that you cannot call note() function after fatal().

That works good generally except one place:

template <class ELFT>
static void reportDuplicate(SymbolBody *Existing,
                            InputSectionBase<ELFT> *ErrSec,
                            typename ELFT::uint ErrOffset) {
....
  print(toString(ErrSec->getFile()) + ": duplicate symbol '" +
            toString(*Existing) + "'",
        "definition found in " + D->Section->getLocation(D->Value));
  print("'" + toString(*Existing) + "' previous definition was here: " +
            toString(D->Section->getFile()),
        "definition found in " + ErrSec->getLocation(ErrOffset));
}

Previous iteration generated error + 3 notes. Current one generates error+note, error+note.
That is not ideal as generating 2 errors instead of one does not look clean, also them may be
emited far from each other in multithreaded case.

I see next possible solutions:

  1. Change signatures to something like: void error(const Twine &Msg, const std::vector<std::string> &Notes) and pass multiple notes at once. Problem is that is looks not possible to have a vector of Twines.
  1. Implement error(), warn(), fatal() using variardic template for Notes argument. That way we should be able to use const Twine& arguments for notes as usual I think. Something like:
template<typename... Note>  
void error(const Twine& Msg, const Note&... args)  { ... }
  1. Protect print calls in reportDuplicate() with std::lock_guard + Mu That way Mu type needs to be changed to std::recursive_mutex and exported I think.
std::lock_guard<std::reqursive_mutex> Lock(Mu);
print(toString(ErrSec->getFile()) + ": duplicate symbol '" +
          toString(*Existing) + "'",
      "definition found in " + D->Section->getLocation(D->Value));
print("'" + toString(*Existing) + "' previous definition was here: " +
          toString(D->Section->getFile()),
      "definition found in " + ErrSec->getLocation(ErrOffset));
  1. Use multiline note:
       print(toString(ErrSec->getFile()) + ": duplicate symbol '" +
             toString(*Existing) + "'",
         "definition found in " + D->Section->getLocation(D->Value) + 
		 "\nprevious definition was here: " + toString(D->Section->getFile()) +
		 "\ndefinition found in " + ErrSec->getLocation(ErrOffset));

That way it will emit error + single multiline note.

grimar updated this revision to Diff 83954.Jan 11 2017, 4:26 AM
  • Changed signatures to:

void warn(const Twine &Msg, llvm::ArrayRef<std::string> Notes = {});
void error(const Twine &Msg, llvm::ArrayRef<std::string> Notes = {});
void fatal(const Twine &Msg, llvm::ArrayRef<std::string> Notes = {});

That solves the problem mentioned and provides clean output (single error + multiple notes).

ruiu added a comment.Jan 11 2017, 10:52 AM

Generally looking good, a few minor comments.

ELF/Error.cpp
65–66

You don't need this piece of code.

ELF/InputSection.cpp
177

s/, if/. If/

185–190

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.

ELF/Target.cpp
104
test/ELF/aarch64-fpic-abs16.s
6

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.

ruiu added a comment.Jan 11 2017, 12:48 PM

I applied this patch and found that you don't need to pass multiple "notes" to warn/error/fatal. Just one note suffices. If you want to print out a multi-line message, simply pass a string containing newlines.

grimar updated this revision to Diff 84106.Jan 12 2017, 4:54 AM
  • Addressed review comments.
ELF/Error.cpp
65–66

Yes. But now I do.

ELF/InputSection.cpp
177

Done.

185–190

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 :)

test/ELF/aarch64-fpic-abs16.s
6

Done.

ruiu added a comment.Jan 12 2017, 1:37 PM

Your comment makes me wonder why you changed something like foo.c:35: <error message> to foo.c: <error message>\nnote: location is 35 in the first place. The former is a familiar format, no? I don't see a justification on that change.

A problem of this patch is it introduced a "note" message and change a lot of messages at the same time. You want to focus on only one thing at a time for ease of development/code review.

ELF/Error.h
28

Remove

ELF/SymbolTable.cpp
360

{} -> ""

grimar updated this revision to Diff 84533.Jan 16 2017, 2:55 AM
  • Addressed review comments.
  • Updated few tests.

I am bit confused, I supposed patch has expected behavior now:

In D27900#644445, @ruiu wrote:

Your comment makes me wonder why you changed something like foo.c:35: <error message> to foo.c: <error message>\nnote: location is 35 in the first place. The former is a familiar format, no? I don't see a justification on that change.

Familiar for compilers, though problem is that we do not always have source file name and source line number in linker.
Actually I think I am changing:

conflict-debug.s:4: duplicate symbol 'zed'
to
{{.*}}.o: duplicate symbol 'zed'
note: definition found in conflict-debug.s:4 or note: definition found in (.text+0x0)

So error is reported for object file name, then notes are used to expand the location to source/line/section offset if available and reports it.
That is universal way as we always have object file name for 'error' and have different possible or no information to use in 'note'
That makes all error reporting to be consistent I believe. And requires introducing 'notes'

A problem of this patch is it introduced a "note" message and change a lot of messages at the same time. You want to focus on only one thing at a time for ease of development/code review.

So idea of patch not about introducing just one more reporting function, but about modifying existent error reporting to use it in a way described above. I belive this patch focuses exactly on that.

Otherwise would be unclear what to write in 'note' and why to have them ?

ELF/Error.h
28

Done.

ruiu added a comment.Jan 16 2017, 10:36 PM

Did you actually find that the current error message format is useless in some case? In my experience, the current format works pretty well. It prints out enough information to fix an error and not too verbose. If you still want to pursue this change, please split it, so that we can discuss each change instead of as a whole.

In D27900#647823, @ruiu wrote:

Did you actually find that the current error message format is useless in some case? In my experience, the current format works pretty well. It prints out enough information to fix an error and not too verbose. If you still want to pursue this change, please split it, so that we can discuss each change instead of as a whole.

Yes, we can see the current behavior isn't ideal from recent bug reports:
http://bugs.llvm.org/show_bug.cgi?id=32008
http://bugs.llvm.org/show_bug.cgi?id=31901

I think we should treat .s/.c file source locations as "enhancements". At the end of the day we always need to provide completely unambiguous information about the error (object file / archive) IMO.

For example, this is useless (taken from http://bugs.llvm.org/show_bug.cgi?id=32008):

/usr/local/bin/ld.lld: error: /usr/src/lib/libutil/logout.c:46: duplicate symbol 'logout'
/usr/local/bin/ld.lld: error: /usr/src/lib/libutil/logout.c:46: previous definition was here

Even when they aren't the same path, not knowing which actual input file to the linker is still annoying:

/usr/local/bin/ld.lld: error: /usr/src/lib/libc/quad/fixunssfdi.c:52: duplicate symbol '__fixunssfdi'
/usr/local/bin/ld.lld: error: /usr/obj/ports/gcc-4.9.4/gcc-4.9.4/libgcc/libgcc2.c:1434: previous definition was here

I think the example I gave of how Clang handles diagnostics for macros is the one we want to be consistent with (from my example in https://reviews.llvm.org/D27676#622869)

foo.c:4:1: error: redefinition of 'foo'
DEFINITION_OF_FOO
^
foo.c:1:32: note: expanded from macro 'DEFINITION_OF_FOO'
#define DEFINITION_OF_FOO void foo() {}
^
foo.c:3:1: note: previous definition is here
DEFINITION_OF_FOO
^
foo.c:1:32: note: expanded from macro 'DEFINITION_OF_FOO'
#define DEFINITION_OF_FOO void foo() {}
^
1 error generated.

What LLD currently does is more like:

foo.c:1:32: error: redefinition of 'foo'
#define DEFINITION_OF_FOO void foo() {}
^
foo.c:1:32: note: previous definition is here
#define DEFINITION_OF_FOO void foo() {}
^
1 error generated.

Even when they aren't the same path, not knowing which actual input file to the linker is still annoying:

/usr/local/bin/ld.lld: error: /usr/src/lib/libc/quad/fixunssfdi.c:52: duplicate symbol '__fixunssfdi'
/usr/local/bin/ld.lld: error: /usr/obj/ports/gcc-4.9.4/gcc-4.9.4/libgcc/libgcc2.c:1434: previous definition was here

I agree. Linker works with object files and not sources. Reporting errors in sources at first place is confusing IMO.

I think the example I gave of how Clang handles diagnostics for macros is the one we want to be consistent with (from my example in https://reviews.llvm.org/D27676#622869)

foo.c:4:1: error: redefinition of 'foo'
DEFINITION_OF_FOO
^
foo.c:1:32: note: expanded from macro 'DEFINITION_OF_FOO'
#define DEFINITION_OF_FOO void foo() {}
^
foo.c:3:1: note: previous definition is here
DEFINITION_OF_FOO
^
foo.c:1:32: note: expanded from macro 'DEFINITION_OF_FOO'
#define DEFINITION_OF_FOO void foo() {}
^
1 error generated.

What LLD currently does is more like:

foo.c:1:32: error: redefinition of 'foo'
#define DEFINITION_OF_FOO void foo() {}
^
foo.c:1:32: note: previous definition is here
#define DEFINITION_OF_FOO void foo() {}
^
1 error generated.

I think this patch currently implements idea of such output.
(or am I missing something ?)

I think this patch currently implements idea of such output.
(or am I missing something ?)

This patch is much better, which is why I want to see it landed! By "What LLD currently does is more like:" I meant ToT (without this patch).

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.

ELF/InputSection.h
151

Nit: Not sure if the comment needs to change, but it should start "Returns detailed information", without the "a"

test/ELF/libsearch.s
25–26

Should this have a check for "note: location is (.bar+0x0)" or similar?

test/ELF/unresolved-symbols.s
24

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?

grimar updated this revision to Diff 89351.Feb 22 2017, 5:17 AM
  • Addressed review comments.
grimar added inline comments.Feb 22 2017, 5:18 AM
ELF/InputSection.h
151

Fixed.

test/ELF/libsearch.s
25–26

Yes, fixed.

test/ELF/unresolved-symbols.s
24

I think "f" here probably was either mistype or a placeholder to trigger testcase fail to get the proper line, which passed by mistake and was forgotten.
Fixed.

ruiu added a comment.Feb 22 2017, 2:00 PM

I don't understand why need "note:" labels in so many places. Why don't you just make error messages multi-line? I don't think there's no such rule that you need to put "note:" after every "\n".

In addition to that, calling error() more than once for one error is not good. That function should be called exactly once for one error.

ruiu added a comment.Feb 22 2017, 2:03 PM

Why it took so much time to land this patch is because it started as a patch in the first place. George, if you want to change the error message format, you should have started it as an RFC, rather than an actual code change. Discussing based on diffs of actual code and test cases is not the best way for both you as a person to propose something new and others who tries to understand what you are trying to do from diffs.

In D27900#683999, @ruiu wrote:

In addition to that, calling error() more than once for one error is not good. That function should be called exactly once for one error.

Does this patch do that ? Where ?

ruiu added a comment.Feb 22 2017, 2:09 PM

Quote from the patch comment:

Example of new output:
error: file-dbg2.o: duplicate symbol 'zed'
error: file-dbg2.a(file-dbg2.o): previous definition was here
note: definitions found in conflict-debug.s(.text+0x0) and conflict-debug.s(.text+0x0)
grimar edited the summary of this revision. (Show Details)Feb 22 2017, 2:17 PM
In D27900#684014, @ruiu wrote:

Quote from the patch comment:

Example of new output:
error: file-dbg2.o: duplicate symbol 'zed'
error: file-dbg2.a(file-dbg2.o): previous definition was here
note: definitions found in conflict-debug.s(.text+0x0) and conflict-debug.s(.text+0x0)

It was really outdated. Patch does not do that atm. Updated the comment.

In D27900#683999, @ruiu wrote:

I don't understand why need "note:" labels in so many places. Why don't you just make error messages multi-line? I don't think there's no such rule that you need to put "note:" after every "\n".

Off the top of my head:

  1. the "note" is colored and is visually distinctive (consider if the message contains a very long file path, and the user is in a narrow terminal so that lines are wrapped; colored "note:" and "error:" are then a visual guide).
  1. Doing it this way is highly structured. If a message contains a location (input object, or source location), it always is the first thing following a "note:" or "error:". This keeps it consistent and unambiguous. Users always know where to look for locations. Again, file paths should be assumed to be quite long and so text containing something like conflict in /some/long/path/foo.o and /some/long/path/bar.o gets hard to parse (have to scan for "and", possibly through line wrapping done by the terminal)

In addition to that, calling error() more than once for one error is not good. That function should be called exactly once for one error.

In D27900#684000, @ruiu wrote:

Why it took so much time to land this patch is because it started as a patch in the first place. George, if you want to change the error message format, you should have started it as an RFC, rather than an actual code change. Discussing based on diffs of actual code and test cases is not the best way for both you as a person to propose something new and others who tries to understand what you are trying to do from diffs.

I agree. We should have a focused discussion for what our "duplicate symbol" error message should look like. The actual code changes are not very complicated compared to the discussion of what we want the end user to see.

rafael edited edge metadata.Mar 14 2017, 2:40 PM

I quite like this. The summary as I see so far

  • Always reporting the .o file is a big improvement, since we might not have the source file.
  • Always reporting the .a file is also important.
  • Having a note be different from the error is nice in that it can be colored differently.

Rui, do you have any objections?

ruiu added a comment.Mar 23 2017, 3:31 PM

I sent a proposal to llvm-dev. Please search for "[RFC] better link error messages". Let's discuss there.

grimar abandoned this revision.Mar 30 2017, 1:31 AM