[llvm-dev] LLD bug causing objcopy ELF to binary generation to create large binaries

[Fangrui Song via llvm-dev]

On 2020-03-30, Andrew Ng via llvm-dev wrote:
>Hi Kasun,
>
>Great that you have already found a fix for your issue. The commitSection
>method would only be called if the output section actually contains an
>input section. So for "empty" output sections the "noload" was not having
>any effect. This is basically what D76981 fixes.
>
>Thanks.
>
>Regards,
>Andrew
>
>On Mon, 30 Mar 2020 at 12:57, Kasun Fernando <kasunf at blackmagicdesign.com>
>wrote:
>
>> Hi Andrew,
>>
>> Thanks for the background and context.
>>
>> "In your issue, just to clarify, is the ELF output from LLD also "large",
>> or is it just the output from the llvm-objcopy operating on that ELF that
>> is "large"? Do you have a simple sample to demonstrate this issue?"
>>
>> The ELF size is actually smaller, compared to what was generated from LLVM
>> 7.x. (~900Kb vs ~250Kb)
>>
>> When we run llvm-objcopy -O Binary blah.elf blah.bin, it would generate a
>> 400Mb binary file.
>>
>>
>> I presume the `noload` flag used in `InputSection` is not properly
>> enforced throughout the code. CommitSection is not applied to .heap and
>> .stack section as I can recall during my debug....
>>
>> .stack (NOLOAD) : ALIGN(4) {
>>         _StackLow = .;
>>         . += _STACK_SIZE; /* Multiple of 4, asserted above. */
>>         _StackHigh = .;
>>
>>         _StackLowIrq = .;
>>         . += _IRQ_STACK_SIZE; /* Multiple of 4, asserted above. */
>>         _StackHighIrq = .;
>>     } > Memory_SRAM
>> ..../* Finally we do the heap */
>>     .heap (NOLOAD) : ALIGN(4) {
>>         ASSERT((_HEAP_SIZE % 4 == 0), "Error: _HEAP_SIZE must be a multiple of 4.");
>>         _HeapLow = .;
>>         /* Use as much space as we can for the heap */
>>         . = ORIGIN(Memory_SRAM) + LENGTH(Memory_SRAM);
>>         _HeapHigh = .;
>>         ASSERT((_HeapLow + _HEAP_SIZE < _HeapHigh), "Error: Not enough space for the heap: data section too big.");
>>     } > Memory_SRAM
>>
>>
>>
>> void OutputSection::commitSection(InputSection *isec) {
>>   if (!hasInputSections) {
>>     // If IS is the first section to be added to this section,
>>     // initialize type, entsize and flags from isec.
>>     hasInputSections = true;
>>     type = isec->type;
>>     entsize = isec->entsize;
>>     flags = isec->flags;
>>   } else {
>>     // Otherwise, check if new type or flags are compatible with existing ones.
>>     unsigned mask = SHF_TLS | SHF_LINK_ORDER;
>>     if ((flags & mask) != (isec->flags & mask))
>>       error("incompatible section flags for " + name + "\n>>> " + toString(isec) +
>>             ": 0x" + utohexstr(isec->flags) + "\n>>> output section " + name +
>>             ": 0x" + utohexstr(flags));
>>     if (type != isec->type) {
>>       if (!canMergeToProgbits(type) || !canMergeToProgbits(isec->type))
>>         error("section type mismatch for " + isec->name + "\n>>> " +
>>               toString(isec) + ": " +
>>               getELFSectionTypeName(config->emachine, isec->type) +
>>               "\n>>> output section " + name + ": " +
>>               getELFSectionTypeName(config->emachine, type));
>>       type = SHT_PROGBITS;
>>     }
>>   }
>>   if (noload)
>>     type = SHT_NOBITS;
>>
>>
>> However, having said that, I think the following fix (although not the
>> cleanest nor addresses any shortcommings in other parts of the code)  seems
>> to fix my issue.
>> https://reviews.llvm.org/D76981
>>
>>
>> I'm also hoping to get involve in the LLVM development in near future.....
>>
>>
>> regards
>> Kasun
>>
>>
>> On 30/3/20 9:50 pm, Andrew Ng wrote:
>>
>> Hi Kasun,
>>
>> The purpose of commit ccba42c7eb3cdfe7824cd4b473a9688e5738fa3a was to fix
>> an issue that was causing incorrect segment file offset alignment for any
>> non-empty segment that happens to start with a section that only contains
>> symbols and no other content. If you look at the test case
>> "ELF/linkerscript/symbol-only-align.test" that might help demonstrate the
>> situation. This particular issue actually resulted in invalid ELF output.
>>
>> I don't remember all the details, but at the time, this was the simplest
>> fix given the code at that point. The alternatives would have required more
>> significant and riskier changes, and it was a relatively urgent fix given
>> that it was producing invalid ELF output.
>>
>> In your issue, just to clarify, is the ELF output from LLD also "large",
>> or is it just the output from the llvm-objcopy operating on that ELF that
>> is "large"? Do you have a simple sample to demonstrate this issue?
>>
>> Thank you.
>>
>> Regards,
>> Andrew

The original issue which motivated D60131 "[ELF] Change default output section type to SHT_PROGBITS"
appeared to have been fixed by a subsequent change.

Keeping the default output section type as SHT_PROGBITS still seems fine
to me, because it is a more consistent rule, and a user can use `(NOLOAD)`
to restore the SHT_NOBITS behavior.