This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Move --print-map(-M)/--cref before checkSections() and openFile()
ClosedPublic

Authored by MaskRay on Mar 10 2020, 4:11 PM.

Details

Summary

-M output can be useful when diagnosing an "error: output file too large" problem (emitted in openFile()).

I just ran into such a situation where I had to debug an erronerous
Linux kernel linker script. It tried to create a file larger than
INT64_MAX bytes.

This patch could have helped https://bugs.llvm.org/show_bug.cgi?id=44715 as well.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 10 2020, 4:11 PM
MaskRay updated this revision to Diff 249519.Mar 10 2020, 4:32 PM
MaskRay retitled this revision from [ELF] Move --print-map(-M)/--cref before openFile() to [ELF] Move --print-map(-M)/--cref before checkSections() and openFile().

Move -M/--cref before checkSections()

grimar added inline comments.Mar 10 2020, 11:48 PM
lld/test/ELF/linkerscript/output-too-large.s
7–28

I'd add comments for each test case.

8

MAP1,CHECK?

13

The "align" calues are misaligned vertically.

psmith added inline comments.Mar 11 2020, 4:55 AM
lld/ELF/Writer.cpp
598

In general I think moving the map file before writing is a good thing, especially when trying to debug problems. Arm's proprietary linker has done something similar to this for a while, it will attempt to print the map file whenever an error has been detected, once a certain stage in the link has passed.
My experience from that:

  • There are sometimes problems when the map file crashes due some information not being available at the time, or not being valid. I don't think the risk is high with this implementation. Is it possible checkSections() might find something that would crash the map file printer? Does the map file writing code need to guard against invalid sections for example?
  • It sometimes gives the impression that the link has succeeded even when it has failed, especially if the error message comes before the map file. If it isn't obvious the link has failed we may need to make it so.
MaskRay marked 3 inline comments as done.Mar 11 2020, 11:53 AM
MaskRay added inline comments.
lld/ELF/Writer.cpp
598

Thanks for sharing the information about Arm's proprietary linker!

There are sometimes problems when the map file crashes due some information not being available at the time, or not being valid.

After finalizeAddressDependentContext() (addresses and symbol assignments) and assignFileOffsets (file offsets), all the information for -M is available. test/ELF/map-file.s and test/ELF/linkerscript/map-file.s make sure we don't run into a missing information problem.

checkSections() just checks whether sections overlap or exceed the address space limit for ELFCLASS32. It does not make any field of a section/symbol assignment invalid. It is safe to move -M before checkSections().

It sometimes gives the impression that the link has succeeded even when it has failed, especially if the error message comes before the map file. If it isn't obvious the link has failed we may need to make it so.

The error status makes it clear the the link has failed. In addition, the error message is still kept.

lld/test/ELF/linkerscript/output-too-large.s
8

error: output file too large is not printed for this test.

13

This is how it gets printed:/

For an ELFCLASS32 output, lld thinks 8 characters are sufficient for an address. Here the address gets overflowed.

MaskRay added a comment.EditedMar 11 2020, 12:00 PM

I think Peter's suggestion is whether (1) we should do an automatic -M when the link fails (2) where we should do -M.

We could move -M earlier further, just after finalizeSections(), but these steps between finalizeSections() and checkSections rarely calls error.
I am still hesitant making -M an automatic thing. It can get in the way if I am diagnosing a problem with -t -y etc, then we will also need an option to turn off the automatic -M. We have --reproduce, so reproducing a link is really easy. I currently don't think an automatic -M is necessary.

MaskRay updated this revision to Diff 249721.Mar 11 2020, 12:12 PM

Add comments to tests

I'm not suggesting we make -M display on every error. It was just mentioning experience of a linker that does do that when the map file is enabled, which approximates what you have here. I think this is good logical step to take, and if we need to alter it later after we gain experience we can do.

So, is the patch good to go as-is? :)

So, is the patch good to go as-is? :)

I'm happy, but better let George approve as he had some suggestions.

grimar accepted this revision.Mar 12 2020, 3:22 AM

LGTM with 2 nits.

lld/test/ELF/linkerscript/output-too-large.s
7–28

Perhaps, "..if an address is greater..."?
I think an address always _can_, but we report the case when it _is_ :)

8

Then you should use --check-prefix, not --check-prefixes.

This revision is now accepted and ready to land.Mar 12 2020, 3:22 AM
MaskRay updated this revision to Diff 249938.Mar 12 2020, 7:58 AM
MaskRay marked 2 inline comments as done.

Address comments

MaskRay marked 2 inline comments as done.Mar 12 2020, 7:59 AM
This revision was automatically updated to reflect the committed changes.