This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Try to not emit weird diagnostics on undefined symbols
ClosedPublic

Authored by davide on Oct 9 2017, 7:29 PM.

Diff Detail

Event Timeline

davide created this revision.Oct 9 2017, 7:29 PM
ruiu added inline comments.Oct 9 2017, 7:38 PM
ELF/Relocations.cpp
865–866

Can you just continue without checking for ErrorCount?

davide added inline comments.Oct 9 2017, 7:42 PM
ELF/Relocations.cpp
865–866

Not quite :(


Testing Time: 2.76s


Failing Tests (36):

ruiu added inline comments.Oct 9 2017, 7:52 PM
ELF/Relocations.cpp
865–866

Why are they failing?

davide added inline comments.Oct 9 2017, 7:56 PM
ELF/Relocations.cpp
865–866

Because recordUndefined is not a function returning a boolean which communicates to the caller whether the reported symbol was actually undefined, it just does side effect calling error (or warn).

grimar added a subscriber: grimar.Oct 10 2017, 12:47 AM
grimar added inline comments.
test/ELF/pr34872.s
8

Testing full error message with NOT is probably brittle and
can make check useless if we change message.
I would just check we have no other errors about symbol:
#CHECK-NOT: fmod

davide added inline comments.Oct 10 2017, 12:58 AM
test/ELF/pr34872.s
8

it's unlikely this message will change imho.

jhenderson added inline comments.Oct 10 2017, 2:35 AM
test/ELF/pr34872.s
8

+1 for @grimar's suggestion. We cannot predict how the error messages might change in the future: even if it's unlikely, it's still possible that they might for some reason (e.g. we might decide to do a wholesale change in their structure, like the move to change to multi-line errors for duplicate symbols).

Fine, I'll change the test. Any other comments or this can go in once the test is fixed? @ruiu ?

ruiu accepted this revision.Oct 10 2017, 10:24 PM

LGTM

This revision is now accepted and ready to land.Oct 10 2017, 10:24 PM
This revision was automatically updated to reflect the committed changes.