Page MenuHomePhabricator

[ELF] - Do not remove empty sections referenced in LOADADDR/ADDR commands.
Needs ReviewPublic

Authored by grimar on Nov 16 2018, 2:28 AM.

Details

Summary

This is https://bugs.llvm.org//show_bug.cgi?id=38750.

If script references empty sections in LOADADDR/ADDR commands

.empty  : { *(.empty ) }
.text   : AT(LOADADDR (.empty) + SIZEOF (.empty)) { *(.text) }

then an empty section will be removed and LOADADDR/ADDR will evaluate to null.
It is not that user may expect from using of the generic script, what is a common case.

There is a trivial fix for that, so I would do that.

Diff Detail

Event Timeline

grimar created this revision.Nov 16 2018, 2:28 AM
ruiu added a comment.Nov 29 2018, 11:12 AM

I really don't like to add more code to the linker script support to make it more and more compatible with GNU, as there is not official language spec and there will always be an incompatibility. How much important is this patch? I think you should discuss that more before start trying to fix. We should not try to "fix" all the reported "issues".

grimar added a reviewer: psmith.EditedNov 30 2018, 1:07 AM

+Peter, interesting to know his opinion about how much useful this one might be (or not) for embedded soft.

We already don't remove an empty section if its expression contains symbols. It seems consistent step to keep the referenced sections as well ten.
We do produce a broken output here now. Personally, I am not a user of tricky linker script features, but I think I am observing such constructions like
AT(ADDR (.section_name) + 0xABC) often in scripts in the wild. There is a difference in how do we handle the empty sections in LLD with what bfd do.
Some differences probably will never be fixed, but some can be fixed easily. I do not understand why we would want to leave the situation (fixed by this patch) "as is" when we:

  1. Already spent some time on fixing the others different cases relative to empty sections to improve consistency with the bfd.
  2. Silently produce a broken output now and reporting the error would be more complicated than actual fix perhaps.
  3. Have a user who faced and reported the issue (and an unknown number of people who did not yet) and a patch that is trivial and consistent with the overall LLD story about empty sections.

I think that this type of script is common in embedded systems. A company will have a product family represented by a common core + variable feature sets, they often have a single linker script that is shared between the projects. This often results in some Output Sections being empty for some of the products. On larger projects it can be difficult to know when this will occur, for example all it takes is one developer to remove the use of a library to silently break a working program.

For this particular example, I don't think it is necessary to keep the .empty section in the ELF file; however I think that it is important for the calculation of the addresses of .text and .data to be either correct or we give an error message. It may be that forcibly keeping .empty is the easiest way of achieving that. An error message is not particularly friendly but it would at least alert the user that they would need to add zero sized sections or symbols to make sure the section was kept.

For this particular example, I don't think it is necessary to keep the .empty section in the ELF file; however I think that it is important for the calculation of the addresses of .text and .data to be either correct or we give an error message. It may be that forcibly keeping .empty is the easiest way of achieving that. An error message is not particularly friendly but it would at least alert the user that they would need to add zero sized sections or symbols to make sure the section was kept.

Thanks, Peter! I suddenly noticed that seems it would be not hard to report an error instead. I'll try to prepare the alternative patch doing that to check what is needed.

This could also occur with ALIGNOF and SIZEOF. It looks like Script->getOrCreateSectionName() is only called when a part of a script references another, whereas SECTIONS uses Script->createSectionName(). Might it be better to set ForceKeepIfEmpty in that function.

This could also occur with ALIGNOF and SIZEOF. It looks like Script->getOrCreateSectionName() is only called when a part of a script references another, whereas SECTIONS uses Script->createSectionName(). Might it be better to set ForceKeepIfEmpty in that function.

Note, I have SIZEOF in the test.

I tried to isolate and minimize the effect, so to keep the empty section only when/if it is really needed.
ALIGNOF and SIZEOF would return 0 for an empty removed section, that seems to be OK just to ignore them.