This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Improve error message for incompatible section types
ClosedPublic

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

Details

Summary

Previously we were not printing out the type 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: section type mismatch for .shstrtab
>>> <internal>:(.shstrtab): SHT_STRTAB
>>> output section .shstrtab: Unknown

Event Timeline

arichardson created this revision.Apr 25 2017, 7:32 AM
ruiu added inline comments.Apr 25 2017, 7:53 AM
ELF/OutputSections.cpp
313

We generally avoid C macros even if it means we need to repeat code multiple times. Please expand this macro.

315

It feels to me that this should live somewhere in libObject or libSupport, so that you can use this both from LLD and llvm-readobj.

324

Do you really want to fall through?

Start message with lower case letter

arichardson marked an inline comment as done.

change error message format and remove macro

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

Can you move this to llvm/Object/ELF.cpp? We already have this function in that file, and I think can do the same.

StringRef getELFRelocationTypeName(uint32_t Machine, uint32_t Type);
arichardson marked an inline comment as done.Apr 25 2017, 9:02 AM
arichardson added inline comments.
ELF/OutputSections.cpp
312

Will do. Should I open a new review for that or add it to this one?

ruiu added inline comments.Apr 27 2017, 11:13 AM
ELF/OutputSections.cpp
456

I think you can return S since StringRef can be converted to std::string implicitly, no?

test/ELF/incompatible-section-types2.s
7–8

Remove extra newlines.

arichardson marked 4 inline comments as done.

Addressed review comments

arichardson updated this revision to Diff 97421.May 2 2017, 3:46 AM
arichardson edited the summary of this revision. (Show Details)

Adapted to D32587 changes

emaste added a subscriber: emaste.May 2 2017, 8:34 AM
ruiu added inline comments.May 2 2017, 10:27 AM
ELF/OutputSections.cpp
472

I'm not sure if you want printing "Unknown" with a hex code, as I think it is rare to see unknown sections in object files. If you don't do that, you can directly use getELFSectionTypeName here. I'd think I prefer that over adding a new function.

arichardson updated this revision to Diff 97611.May 3 2017, 5:12 AM
arichardson marked an inline comment as done.
arichardson edited the summary of this revision. (Show Details)

Remove helper function and call getELFSectionTypeName directly

ELF/OutputSections.cpp
472

As we now know which object file section it is we can just look at it to get the hex value so I agree we don't need it. The other option would be to always print the hex value (like llvm-readobj does).

ruiu accepted this revision.May 5 2017, 2:15 PM

LGTM with these changes. Thank you for doing this!

ELF/OutputSections.cpp
473

I think you can remove object:: from this line

475

and this.

This revision is now accepted and ready to land.May 5 2017, 2:15 PM
arichardson updated this revision to Diff 98054.May 6 2017, 5:52 AM
arichardson marked 4 inline comments as done.

removed object::

I don't have commit access, could you please commit for me?

This revision was automatically updated to reflect the committed changes.