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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 27 07:40:38 PDT 2019


MaskRay added inline comments.


================
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);
----------------
peter.smith wrote:
> 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().
We cannot guarantee getOutputSection() returns non nullptr.

merge-gc-piece2.s and gc-sections-non-alloc-to-merge.s are tests where a MergeInputSection may have a null InputSection parent.

I tried adding more asserts to SectionBase::getOutputSection but that seems hard. We probably should do some other layout changes before enforcing nonnullness in getOutputSection.

Restored with an assert.


================
Comment at: test/ELF/linkerscript/icf-different-output-sections.s:45
+.section .rodata.bar1,"a"
+.byte 42
----------------
grimar wrote:
> Seems you only need to have a 2 .rodata sections for this test?
> 
> I'd suggest testing/demonstrating the output from the 2 scripts here probably:
> 
> 1) .rodata.foo : { *(.rodata.foo*) } .rodata.bar : { *(.rodata.bar*) }
> 
> 2) .rodata: { *(.rodata.*) } 
> 
> I.e. idea is to show in the test that we can put sections in a single or a multiple sections and 
> that in the case (2) input sections will be folded and in case (1) they will not.
I think .rodata* are redundant. I will delete them.

Will add another script for your point (1), and add another script for orphans.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D66717





More information about the llvm-commits mailing list