Kasun Fernando via llvm-dev
2020-Mar-29 21:50 UTC
[llvm-dev] LLD bug causing objcopy ELF to binary generation to create large binaries
Hi LLVM devs, I came across an LLD bug in v 10.x where ELF parser / processor is setting .PROGBITS attribute for .heap and .stack sections, which leads to large binaries when we do `llvm-objcopy -o binary` to generate the binary output for armv6m. (e.g. for a 57Kb elf would yield a ~400Mb binary). This in comparison with LLVM 7.x , would produce the correct binary size of 35Kb and the elf sections have NOBITS for .heap and .stack sections. I narrowed down the problem to the following commit and the commits around this....Please see below: commit ccba42c7eb3cdfe7824cd4b473a9688e5738fa3a Author: Andrew Ng <anng.sw at gmail.com> Date: Tue Apr 23 12:38:52 2019 +0000 [ELF] Change default output section type to SHT_PROGBITS This fixes an issue where a symbol only section at the start of a PT_LOAD segment, causes incorrect alignment of the file offset for the start of the segment which results in the output of an invalid ELF. SHT_PROGBITS was the default output section type in the past. Differential Revision: https://reviews.llvm.org/D60131 llvm-svn: 358981 commit 5929553868ddfd3f53672253782260c2a0a52c79 Author: Fangrui Song <maskray at google.com> Date: Wed Apr 24 14:44:07 2019 +0000 [ELF] Delete a redundant SHT_NOBITS -> SHT_PROGBITS after D60131 Differential Revision: https://reviews.llvm.org/D61006 llvm-svn: 359099 What was the intention behind "Andrew Ng"'s commit? was it an oversight to set PROGBITS attribute unconditionally for sections that are allocation only? regards Kasun
Andrew Ng via llvm-dev
2020-Mar-30 10:50 UTC
[llvm-dev] LLD bug causing objcopy ELF to binary generation to create large binaries
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 On Sun, 29 Mar 2020 at 22:50, Kasun Fernando <kasunf at blackmagicdesign.com> wrote:> Hi LLVM devs, > > > I came across an LLD bug in v 10.x where ELF parser / processor is > setting .PROGBITS attribute for .heap and .stack sections, which leads > to large binaries when we do `llvm-objcopy -o binary` to generate the > binary output for armv6m. (e.g. for a 57Kb elf would yield a ~400Mb > binary). > > This in comparison with LLVM 7.x , would produce the correct binary size > of 35Kb and the elf sections have NOBITS for .heap and .stack sections. > > > I narrowed down the problem to the following commit and the commits > around this....Please see below: > > > commit ccba42c7eb3cdfe7824cd4b473a9688e5738fa3a > Author: Andrew Ng <anng.sw at gmail.com> > Date: Tue Apr 23 12:38:52 2019 +0000 > > [ELF] Change default output section type to SHT_PROGBITS > > This fixes an issue where a symbol only section at the start of a > PT_LOAD segment, causes incorrect alignment of the file offset for the > start of the segment which results in the output of an invalid ELF. > > SHT_PROGBITS was the default output section type in the past. > > Differential Revision: https://reviews.llvm.org/D60131 > > llvm-svn: 358981 > > > commit 5929553868ddfd3f53672253782260c2a0a52c79 > Author: Fangrui Song <maskray at google.com> > Date: Wed Apr 24 14:44:07 2019 +0000 > > [ELF] Delete a redundant SHT_NOBITS -> SHT_PROGBITS after D60131 > > Differential Revision: https://reviews.llvm.org/D61006 > > llvm-svn: 359099 > > > What was the intention behind "Andrew Ng"'s commit? was it an oversight > to set PROGBITS attribute unconditionally for sections that are > allocation only? > > > regards > > Kasun > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200330/92e36d2f/attachment-0001.html>
Kasun Fernando via llvm-dev
2020-Mar-30 11:57 UTC
[llvm-dev] LLD bug causing objcopy ELF to binary generation to create large binaries
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
>
> On Sun, 29 Mar 2020 at 22:50, Kasun Fernando
> <kasunf at blackmagicdesign.com <mailto:kasunf at
blackmagicdesign.com>> wrote:
>
> Hi LLVM devs,
>
>
> I came across an LLD bug in v 10.x where ELF parser / processor is
> setting .PROGBITS attribute for .heap and .stack sections, which
> leads
> to large binaries when we do `llvm-objcopy -o binary` to generate the
> binary output for armv6m. (e.g. for a 57Kb elf would yield a ~400Mb
> binary).
>
> This in comparison with LLVM 7.x , would produce the correct
> binary size
> of 35Kb and the elf sections have NOBITS for .heap and .stack
> sections.
>
>
> I narrowed down the problem to the following commit and the commits
> around this....Please see below:
>
>
> commit ccba42c7eb3cdfe7824cd4b473a9688e5738fa3a
> Author: Andrew Ng <anng.sw at gmail.com <mailto:anng.sw at
gmail.com>>
> Date: Tue Apr 23 12:38:52 2019 +0000
>
> [ELF] Change default output section type to SHT_PROGBITS
>
> This fixes an issue where a symbol only section at the start of a
> PT_LOAD segment, causes incorrect alignment of the file
> offset for the
> start of the segment which results in the output of an
> invalid ELF.
>
> SHT_PROGBITS was the default output section type in the past.
>
> Differential Revision: https://reviews.llvm.org/D60131
>
> llvm-svn: 358981
>
>
> commit 5929553868ddfd3f53672253782260c2a0a52c79
> Author: Fangrui Song <maskray at google.com <mailto:maskray at
google.com>>
> Date: Wed Apr 24 14:44:07 2019 +0000
>
> [ELF] Delete a redundant SHT_NOBITS -> SHT_PROGBITS after
D60131
>
> Differential Revision: https://reviews.llvm.org/D61006
>
> llvm-svn: 359099
>
>
> What was the intention behind "Andrew Ng"'s commit? was
it an
> oversight
> to set PROGBITS attribute unconditionally for sections that are
> allocation only?
>
>
> regards
>
> Kasun
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200330/5f78eb80/attachment-0001.html>