[PATCH] D38361: [ELF] Stop setting output section size early

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks!

James Henderson via Phabricator <reviews at reviews.llvm.org> writes:

> jhenderson updated this revision to Diff 126347.
> jhenderson added a comment.
>
>> @rafael wrote:
>>  I don't like the idea of adding a member for the section position. At no point we need the position and the offset simultaneously and having both variables means they can get inconsistent.
>
> As far as I'm aware there is no case where the two could get inconsistent, once set - that would require the ability to reorder the sections in the output section.
>
>> One way to look at the current issue is that the early estimate of the offset is correct in the common case, which makes it really easy to use it incorrectly before the final value is computed.
>
> I agree that using a value that is very different is likely to be spotted, although it's not guaranteed to be (especially if working with only a single InputSection in an output section).
>
>> If we make the value almost always wrong, that problem goes away. What do you think of the attached patch? We could rename OutSecOff to OutSecOffsetOrIndex, but that doesn't seem worth it.
>
> Given that both you and @ruiu seem reluctant to have new members, I've tentatively adopted it with a few changes and additions to the comments. I don't like using a member for two different purposes still, even if those two purposes don't overlap in time, but I'd rather get this change in in at least some form. Could you let me know what you think of the diff with the comment changes, please. In particular, I've moved the comments relating to the Size and OutSecOff members to make their dual purpose clear at their point of definition, and I've not added the suggested additional comments to maybeCompress - they're unnecessary as we already have a comment in assignOffsets saying that we don't support custom layout for compressed debug sections.
>
>>> I'm wondering if it is feasible to check and disable compression if a non input section description command is detected in the debug section being compressed?
>> 
>> It should be possible to at least produce an error message.
>
> I'd support something like that (it should be easy to add to the loop I'm adding in maybeCompress), but not as part of this change.
>
>
> https://reviews.llvm.org/D38361
>
> Files:
>   ELF/InputSection.h
>   ELF/LinkerScript.cpp
>   ELF/OutputSections.cpp
>   ELF/OutputSections.h
>   test/ELF/linkerscript/unused-synthetic.s
>
> Index: test/ELF/linkerscript/unused-synthetic.s
> ===================================================================
> --- test/ELF/linkerscript/unused-synthetic.s
> +++ test/ELF/linkerscript/unused-synthetic.s
> @@ -13,6 +13,16 @@
>  # CHECK:      .text
>  # CHECK-NEXT: .dynsym
>  
> +# Test that the size of a removed unused synthetic input section is not added
> +# to the output section size. Adding a symbol assignment prevents removal of
> +# the output section, but does not cause the section size to be recomputed.
> +# RUN: echo "SECTIONS { \
> +# RUN:    .got.plt : { a_sym = .; *(.got.plt) } \
> +# RUN:  }" > %t2.script
> +# RUN: ld.lld -shared -o %t2.so --script %t2.script %t.o
> +# RUN: llvm-objdump -section-headers %t2.so | FileCheck %s --check-prefix=CHECK2
> +# CHECK2: .got.plt 00000000
> +
>  .global _start
>  _start:
>    nop
> Index: ELF/OutputSections.h
> ===================================================================
> --- ELF/OutputSections.h
> +++ ELF/OutputSections.h
> @@ -71,8 +71,12 @@
>    // it may have a non-null value.
>    OutputSection *RelocationSection = nullptr;
>  
> -  // The following fields correspond to Elf_Shdr members.
> +  // Initially this field is the number of InputSections that have been added to
> +  // the OutputSection so far. Later on, after a call to assignAddresses, it
> +  // corresponds to the Elf_Shdr member.
>    uint64_t Size = 0;
> +
> +  // The following fields correspond to Elf_Shdr members.
>    uint64_t Offset = 0;
>    uint64_t LMAOffset = 0;
>    uint64_t Addr = 0;
> Index: ELF/OutputSections.cpp
> ===================================================================
> --- ELF/OutputSections.cpp
> +++ ELF/OutputSections.cpp
> @@ -116,13 +116,7 @@
>    IS->Parent = this;
>    Flags |= IS->Flags;
>    Alignment = std::max(Alignment, IS->Alignment);
> -
> -  // The actual offsets will be computed by assignAddresses. For now, use
> -  // crude approximation so that it is at least easy for other code to know the
> -  // section order. It is also used to calculate the output section size early
> -  // for compressed debug sections.
> -  IS->OutSecOff = alignTo(Size, IS->Alignment);
> -  this->Size = IS->OutSecOff + IS->getSize();
> +  IS->OutSecOff = Size++;
>  
>    // If this section contains a table of fixed-size entries, sh_entsize
>    // holds the element size. If it contains elements of different size we
> @@ -189,6 +183,15 @@
>        !Name.startswith(".debug_"))
>      return;
>  
> +  // Calculate the section offsets and size pre-compression.
> +  Size = 0;
> +  for (BaseCommand *Cmd : SectionCommands)
> +    if (auto *ISD = dyn_cast<InputSectionDescription>(Cmd))
> +      for (InputSection *IS : ISD->Sections) {
> +        IS->OutSecOff = alignTo(Size, IS->Alignment);
> +        this->Size = IS->OutSecOff + IS->getSize();
> +      }
> +
>    // Create a section header.
>    ZDebugHeader.resize(sizeof(Elf_Chdr));
>    auto *Hdr = reinterpret_cast<Elf_Chdr *>(ZDebugHeader.data());
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -674,6 +674,11 @@
>    if (Ctx->OutSec->Flags & SHF_COMPRESSED)
>      return;
>  
> +  // The Size previously denoted how many InputSections had been added to this
> +  // section, and was used for sorting SHF_LINK_ORDER sections. Reset it to
> +  // compute the actual size value.
> +  Sec->Size = 0;
> +
>    // We visited SectionsCommands from processSectionCommands to
>    // layout sections. Now, we visit SectionsCommands again to fix
>    // section offsets.
> Index: ELF/InputSection.h
> ===================================================================
> --- ELF/InputSection.h
> +++ ELF/InputSection.h
> @@ -318,8 +318,10 @@
>  
>    OutputSection *getParent() const;
>  
> -  // The offset from beginning of the output sections this section was assigned
> -  // to. The writer sets a value.
> +  // This variable has two usages. Initially, it represents an index in the
> +  // OutputSection's InputSection list, and is used when ordering SHF_LINK_ORDER
> +  // sections. After assignAddresses is called, it represents the offset from
> +  // the beginning of the output section this section was assigned to.
>    uint64_t OutSecOff = 0;
>  
>    static bool classof(const SectionBase *S);