[prev in list] [next in list] [prev in thread] [next in thread] 

List:       llvm-dev
Subject:    Re: [llvm-dev] LLD bug causing objcopy ELF to binary generation to create large binaries
From:       Fangrui Song via llvm-dev <llvm-dev () lists ! llvm ! org>
Date:       2020-03-31 18:58:13
Message-ID: 20200331185813.ac2wnozhmmwgbe44 () google ! com
[Download RAW message or body]

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@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.
_______________________________________________
LLVM Developers mailing list
llvm-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


[prev in list] [next in list] [prev in thread] [next in thread]