This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Improve error message for incompatible section flags
ClosedPublic

Authored by arichardson on Apr 25 2017, 5:32 AM.

Details

Summary

Previously we were not printing out the flags of the incompatible section which made it difficult to determine what the problem was.

The error message format has been change to the following:

error: incompatible section flags for .bar
>>> /foo/bar/incompatible-section-flags.s.tmp.o:(.bar): 0x403
>>> output section .bar: 0x3

Event Timeline

arichardson created this revision.Apr 25 2017, 5:32 AM
grimar edited edge metadata.Apr 25 2017, 5:57 AM

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

Ah sorry, Sec is an output section here. My suggestion will not work :) Need to think more about it.

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

I think that output format looks better but I don't see how we can determine whichinput file the OutputSection flags are derived from as they are a bitwise or of multiple files.
I guess one option is adding list of InputSections that caused the Flags field to be updated (which would be empty if Sec->Flags == Sec->Sections[0]->Flags)?

Also with very long paths it might make sense to have the flags printed first?

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

I think that output format looks better but I don't see how we can determine whichinput file the OutputSection flags are derived from as they are a bitwise or of multiple files.
I guess one option is adding list of InputSections that caused the Flags field to be updated (which would be empty if Sec->Flags == Sec->Sections[0]->Flags)?

Also with very long paths it might make sense to have the flags printed first?

Yeah, I posted suggestion too early, sorry, it will not work. And we also can't assume that sections has the same names,
I think this function used for script as well. It probably can be something like:

Section .input_section_name has flags (0x403) incompatible with output section .output_section_name flags (0x3).
>> .input_section_name is defined in foo.a(bar.o)
arichardson edited the summary of this revision. (Show Details)

changed error message

Add full stop at end of message.

ruiu added inline comments.Apr 25 2017, 7:58 AM
ELF/OutputSections.cpp
411

Error message usually should start with a lowercase letter and shouldn't end with a period. Reason being that it allows you to construct new error messages from other error messages. Say, when you have a string "section does not exist: foo", then you can construct a new error message by prepending another message, e.g. "error while reading file: section does not exist: foo". (We do not construct error messages from other error messages at the moment, though.)

Start with lowercase letter and remove full stop.

ruiu added inline comments.Apr 25 2017, 8:10 AM
ELF/OutputSections.cpp
407

Remove TODO. This is a nice improvement but not mandatory.

409–412

Do you think this message is in line with other error messages? For example, we have this error message for TLS attribute mismatch:

error: TLS attribute mismatch: foo
>>> defined in xyz.o
>>> defined in abc.o

I'd probably do something like this.

error: section attribute mismatch
>>> abc.o:(.foo): 0x12
>>> xyz.o:(.foo): 0x34

Make error message more like other error messages

ruiu accepted this revision.Apr 25 2017, 9:00 AM

LGTM

This revision is now accepted and ready to land.Apr 25 2017, 9:00 AM
arichardson edited the summary of this revision. (Show Details)Apr 25 2017, 9:06 AM
arichardson marked 3 inline comments as done.

Could you commit for me? I don't have commit access.

This revision was automatically updated to reflect the committed changes.