Page MenuHomePhabricator

ELF: Allow forward references to linked sections.
ClosedPublic

Authored by pcc on Jul 17 2019, 12:42 PM.

Details

Summary

It's possible to create IR that uses !associated to refer to a global that
appears later in the module, which can result in these types of forward
references being generated. Unfortunately our assembler does not currently
accept the resulting .s so I needed to use yaml2obj to test this.

Event Timeline

pcc created this revision.Jul 17 2019, 12:42 PM
Herald added a project: Restricted Project. · View Herald Transcript

The latest semantics of SHF_LINK_ORDER agreed by several parties on generic-abi has the following wording:

SHF_LINK_ORDER 

This flag adds special ordering requirements for link editors. The 
requirements apply to the referenced section identified by the sh_link 
field of this section's header.  If this section is combined with other 
sections in the output file, the section must appear in the same 
relative order with respect to those sections, as the referenced 
section appears with respect to sections the referenced section is 
combined with. 

A typical use of this flag is to build a table that references 
text or data sections in address order. 

In addition to adding ordering requirements, SHF_LINK_ORDER indicates 
that the section contains metadata describing the referenced section. 
When performing unused section elimination, the link editor should 
ensure that both the section and the referenced section are retained 
or discarded together. Furthermore, relocations from this section 
into the referenced section should not be taken as evidence that the 
referenced section should be retained.

Allowing forward references is natural and does not conflict with the wording. LGTM.

lld/ELF/InputFiles.cpp
657

This can be changed to a range-based for.

lld/test/ELF/linkorder-forward-ref.test
5

This test needs a comment explaining what it intends to test.

pcc updated this revision to Diff 210477.Jul 17 2019, 7:29 PM
pcc marked 2 inline comments as done.
  • Add comment
lld/ELF/InputFiles.cpp
657

We're iterating over two arrays here (this->sections and objSections) which makes it simpler to use a regular for loop.

MaskRay accepted this revision.Jul 17 2019, 9:14 PM
This revision is now accepted and ready to land.Jul 17 2019, 9:14 PM
grimar accepted this revision.Jul 18 2019, 12:05 AM

LGTM. A few "up to you" suggestions are below

lld/ELF/InputFiles.cpp
657

nit: we usually use ++i form I think.

lld/test/ELF/linkorder-forward-ref.test
7

nit: for LLVM tools it became common to start comments from ##.
Many tests in LLD also do that (it is actually where this style initially start spreading).
I'd suggest using it.

21

nit: seems you do not need SHF_ALLOC, SHF_EXECINSTR flags here?

peter.smith added inline comments.Jul 18 2019, 2:54 AM
lld/ELF/InputFiles.cpp
656

A possible optimization would be to remember the index of the first SHF_LINK_ORDER section in the previous loop. Then start the second iteration from there. If there were no SHF_LINK_ORDER sections which I would expect to be rare outside of Arm then we would skip the second loop entirely. I wouldn't expect this to be significant though so it may be better to keep it simple.

ruiu accepted this revision.Jul 18 2019, 6:36 AM

LGTM

lld/ELF/InputFiles.cpp
656

I thought that too, but since the section table is an efficient data structure (a vector) and its size is not very large, so I don't think we'll see a difference by caching something in the first loop.

664

Flip the condition to continue early.

This revision was automatically updated to reflect the committed changes.
pcc marked 5 inline comments as done.