[PATCH] D66717: [ELF] Do not ICF two sections with different output sections (by SECTIONS commands)

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I agree that this is going in the right direction. I'm particularly happy about splitting out the symbol assignments.



================
Comment at: ELF/LinkerScript.cpp:53
   if (sec)
-    return alignTo(sec->getOffset(val) + getOutputSectionVA(sec, loc),
+    return alignTo(sec->getOffset(val) + sec->getOutputSection()->addr,
                    alignment);
----------------
If we end up calling this with sec->getOutputSection() == nullptr we'll get a segfault that could be hard to track down. It may be worth preserving getOutputSectionVA with an assert non nullptr. Alternatively if we can guarantee that getOutputSection() is non nullptr then we could put an assert into getOutputSection().


================
Comment at: ELF/Writer.cpp:155
 
-static void copySectionsIntoPartitions() {
+void elf::copySectionsIntoPartitions() {
   std::vector<InputSectionBase *> newSections;
----------------
MaskRay wrote:
> ruiu wrote:
> > Maybe this and `createSynethticSections` should be moved to a new file if they are no longer a part of the writer?
> Do you have a recommendation what the new file should be called? While this is under review, I am experimenting other reordering this change will enable (e.g. moving addOrphanSections from Writer.cpp to between processSectionCommands/ICF). The definition of "Writer" is indeed unclear to me now...
I've not got a good answer right now, will try and have a think. May be worth going through the overall control flow and seeing what comes out. My high-level view of a linker control flow is:
- Load all content from files into an address independent representation.
- Do address independent transformation such as GC and ICF.
- Create synthetic InputSections.
- Layout the output file (InputSections -> OutputSections), although not necessarily assign addresses yet. 
- Populate synthetic content such as the .plt and .got. It helps to have removed as much as possible prior to this point.
- Assign addresses.
- Perfom address dependent transformations.
- Finalize addresses (We now have a logical final ELF file).
- Write ELF file.

At each stage we are adding information, permitting more transformations, but also restricting our freedom as making changes invalidates the information. That is a control flow view of the linker, rather than a structural view of the components though.

I think our Writer.cpp combines everything from create synthetic InputSections down. If I were hazarding a guess at a new name for the moved functions I'd recommend something like layout.cpp. In any case it may be worth taking a step back to see what the options are.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66717/new/

https://reviews.llvm.org/D66717