Page MenuHomePhabricator

[ELF] - Use full object name if source file name exist when reporting errors.
AbandonedPublic

Authored by grimar on Dec 12 2016, 9:08 AM.

Details

Reviewers
ruiu
rafael
Summary

Previously if we were able to take source name, we used it exclusively for reporting.
PR31354 case shows how confusing it can be:

Error is:
/usr/bin/ld: error: byte_copy.c:(.text+0x0): duplicate symbol 'byte_copy'
/usr/bin/ld: error: byte_copy.c:(function byte_copy): previous definition was here

After this patch output includes archive name if any. What makes clear what the error is about:
lld.exe: error: usr/ports/sysutils/safecat/work/safecat-1.13/byte_copy.o(byte_copy.c):(.text+0x0): duplicate symbol 'byte_copy'
lld.exe: error: usr/ports/sysutils/safecat/work/safecat-1.13/str.a(byte_copy.o)(byte_copy.c):(function byte_copy): previous definition was here

Diff Detail

Event Timeline

grimar updated this revision to Diff 81091.Dec 12 2016, 9:08 AM
grimar retitled this revision from to [ELF] - Use full object name if source file name exist when reporting errors..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
ruiu edited edge metadata.Dec 12 2016, 2:21 PM

Isn't it too verbose? Source file names are the thing you need to fix your problems, so it might be better to not print out object files to keep developers from distracting.

With my lld contributor hat off (and with my lld user hat on) I'm 100% in favour of better diagnostics for lld different archive semantics. We've already hit one or two cases in FreeBSD and at least two cases internally where this was an issue. @silvas is very fond of it as I think he was the first one noticing it trying to link a real codebase.

ruiu added a comment.Dec 13 2016, 4:40 PM

I think my concern is that we are going to print out at most three filenames like this

foo.a(foo.o)(foo.c)

and that error string format looks odd. It is indistinguishable from

foo.o(foo.c)

where in this case foo.o is not an archive but an object file. I think we are using too many parentheses.

In D27676#621721, @ruiu wrote:

I think my concern is that we are going to print out at most three filenames like this

foo.a(foo.o)(foo.c)

and that error string format looks odd. It is indistinguishable from

foo.o(foo.c)

where in this case foo.o is not an archive but an object file. I think we are using too many parentheses.

In the post-commit of r285186 I suggested using a note: to report the object file.

Maybe when we use a source location for the error, we can add a note something like:

note: this definition was found in baz.a(bar.o)
In D27676#621721, @ruiu wrote:

I think my concern is that we are going to print out at most three filenames like this

foo.a(foo.o)(foo.c)

and that error string format looks odd. It is indistinguishable from

foo.o(foo.c)

where in this case foo.o is not an archive but an object file. I think we are using too many parentheses.

Speaking about this one "duplicate symbol" issue, as a user I would probably be happy to see something close to Sean suggestion with additional info.

What about if we have next form here:
/usr/bin/ld: error: byte_copy.c:(.text+0x0): duplicate symbol 'byte_copy'
/usr/bin/ld: error: byte_copy.c:(function byte_copy): previous definition was here
note: 'byte_copy' definitions were found both in usr/ports/sysutils/safecat/work/safecat-1.13/byte_copy.o and usr/ports/sysutils/safecat/work/safecat-1.13/str.a(byte_copy.o),
check that you do not include objects files twice.

One more profit from extended messages is that if something is still not clear, user can always google something like "lld check that you do not include objects files twice error".
Special messages can help here to find issues specific for lld and probably make search results more useful.

In D27676#621721, @ruiu wrote:

I think my concern is that we are going to print out at most three filenames like this

foo.a(foo.o)(foo.c)

and that error string format looks odd. It is indistinguishable from

foo.o(foo.c)

where in this case foo.o is not an archive but an object file. I think we are using too many parentheses.

Speaking about this one "duplicate symbol" issue, as a user I would probably be happy to see something close to Sean suggestion with additional info.

What about if we have next form here:
/usr/bin/ld: error: byte_copy.c:(.text+0x0): duplicate symbol 'byte_copy'
/usr/bin/ld: error: byte_copy.c:(function byte_copy): previous definition was here
note: 'byte_copy' definitions were found both in usr/ports/sysutils/safecat/work/safecat-1.13/byte_copy.o and usr/ports/sysutils/safecat/work/safecat-1.13/str.a(byte_copy.o),
check that you do not include objects files twice.

I'm not entirely sure about this diagnostic, and I think the problem is different here (in the general case).
It's not an object file included twice. It's that there are two object files providing a non-weak definition of the same symbol and the order on which you fetch the members from the archive matters.

In D27676#621721, @ruiu wrote:

I think my concern is that we are going to print out at most three filenames like this

foo.a(foo.o)(foo.c)

and that error string format looks odd. It is indistinguishable from

foo.o(foo.c)

where in this case foo.o is not an archive but an object file. I think we are using too many parentheses.

Speaking about this one "duplicate symbol" issue, as a user I would probably be happy to see something close to Sean suggestion with additional info.

What about if we have next form here:
/usr/bin/ld: error: byte_copy.c:(.text+0x0): duplicate symbol 'byte_copy'
/usr/bin/ld: error: byte_copy.c:(function byte_copy): previous definition was here
note: 'byte_copy' definitions were found both in usr/ports/sysutils/safecat/work/safecat-1.13/byte_copy.o and usr/ports/sysutils/safecat/work/safecat-1.13/str.a(byte_copy.o),
check that you do not include objects files twice.

I'm not entirely sure about this diagnostic, and I think the problem is different here (in the general case).
It's not an object file included twice. It's that there are two object files providing a non-weak definition of the same symbol and the order on which you fetch the members from the archive matters.

My wording may be inaccurate. Idea was to place details in note (like suggested by Sean) but also add some reasonable hint about possible reason of issue.

ruiu added a comment.Dec 14 2016, 2:50 PM

I thought about this for a while. I'd say that having foo.a(foo.o)(foo.c) and foo.o(foo.c) are not correct because they are inconsistent. If the former were foo.a(foo.o(foo.c)), the two are at least consistent, but I guess that's hard to read.

We have a lot of combinations here.

  • Is an object file in an archive file? Yes or no
  • Do we know source filename? Yes or not
  • What do we know about the error location inside of the object file: Line number in the original source code, section name in an object file, or nothing

We don't need to print out all the information we know. For example, if we know a line number, we don't need to print out a section name. However, at the same time, we want to print out in a machine-readable format so that it can be integrated to an IDE.

So, that's a little bit complicated. We probably need to come up with a reasonable rule here.

In D27676#622851, @ruiu wrote:

I thought about this for a while. I'd say that having foo.a(foo.o)(foo.c) and foo.o(foo.c) are not correct because they are inconsistent. If the former were foo.a(foo.o(foo.c)), the two are at least consistent, but I guess that's hard to read.

We have a lot of combinations here.

  • Is an object file in an archive file? Yes or no
  • Do we know source filename? Yes or not
  • What do we know about the error location inside of the object file: Line number in the original source code, section name in an object file, or nothing

I think we can keep the source file / line location information completely separate from the object file location information.

If you think of clang, the error: is often consistent with GCC. When clang provides more information (e.g. "did you mean?", "expanded from macro FOO defined here.", ...) it is usually as a note. Maybe we can be consistent with that: we produce a similar diagnostic to existing linkers, but below each error: line, we can add a handy note:.

For example, look at Clang's output for this:
https://godbolt.org/g/GMFHeq

#line 1 "foo.c"
#define DEFINITION_OF_FOO void foo() {}

DEFINITION_OF_FOO
DEFINITION_OF_FOO

The diagnostic is:

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.

I think we can be consistent with that. The error: redefinition of 'foo' and note: previous definition is here would be basically traditional object location linker diagnostics.

The note: expanded from macro 'DEFINITION_OF_FOO' and note: expanded from macro 'DEFINITION_OF_FOO' would be our extended linker diagnostics providing the original source location. But instead of "expanded from macro" they would be a message like note: source code of definition is here

In D27676#622851, @ruiu wrote:

I thought about this for a while. I'd say that having foo.a(foo.o)(foo.c) and foo.o(foo.c) are not correct because they are inconsistent. If the former were foo.a(foo.o(foo.c)), the two are at least consistent, but I guess that's hard to read.

We have a lot of combinations here.

  • Is an object file in an archive file? Yes or no
  • Do we know source filename? Yes or not
  • What do we know about the error location inside of the object file: Line number in the original source code, section name in an object file, or nothing

I think we can keep the source file / line location information completely separate from the object file location information.

I tried to implement that idea in D27900.

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