Page MenuHomePhabricator

[ELF] Warn changed output section address
ClosedPublic

Authored by MaskRay on Feb 17 2020, 3:16 PM.

Details

Summary

When the output section address (addrExpr) is specified, GNU ld warns if
sh_addr is different. This patch implements the warning.

Note, LinkerScript::assignAddresses can be called more than once. We
need to record the changed section addresses, and only report the
warnings when the addresses are finalized.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 17 2020, 3:16 PM
MaskRay marked an inline comment as done.Feb 17 2020, 3:19 PM
MaskRay added inline comments.
lld/test/ELF/linkerscript/section-align2.test
1–3

AArch64 needs thunks. assignAddresses is called at least twice.

This enhances the test - it can detect duplicate diagnostic issue (https://sourceware.org/bugzilla/show_bug.cgi?id=25570 )

Just had some thoughts on what values we warn, and what value we give as the original.

lld/test/ELF/linkerscript/lma-align.test
8

While the warnings look correct from the LLD last known update perspective, are they what a user would want to see? Looking at the script:

  • As a user I set the address of .data to 0x11000, why is LLD reporting 0x11001? Perhaps it is worth reporting the value of the original addrexpr.
  • Although "." is an addrexpr it is no different to not using an addrexpr. Could we consider addrexpr involving "." as relative to previous dot expressions and not warn? I'm not 100% behind not warning as there could be a context like . = 0x10000 ; before the addrexpr involving "." although I expect that is rare.
SECTIONS {
  .text 0x1000 : { *(.text*) } >ROM
  ## Input section alignment decides output section alignment.
  .data.rel.ro 0x11000 : { *(.data.rel.ro) } >RAM AT>ROM
  ## ALIGN decides output section alignment.
  .data . : ALIGN(16) { *(.data*) } >RAM AT>ROM
  ## Start a new PT_LOAD because the LMA region is different from the previous one.
  .bss . : ALIGN(64) { *(.bss*) } >RAM
}
MaskRay marked an inline comment as done.Feb 18 2020, 12:10 PM
MaskRay added inline comments.
lld/test/ELF/linkerscript/lma-align.test
8

This warning is reported only when addrExpr is set (., not ALIGN(16)).

section-align2.test has a case where addrExpr is set while alignExpr is not:

.data2 . : { *(.data2) }

As a user I set the address of .data to 0x11000, why is LLD reporting 0x11001? Perhaps it is worth reporting the value of the original addrexpr.

When lld tries to layout .data, dot is 0x11001.

For comparison, GNU ld reports ld.bfd: warning: changing start of section .data2 by 7 bytes

I believe this patch matches GNU ld behavior: warn if the actual address is not equal to addrExpr. This can be caused by either ALIGN or input section alignments.

MaskRay updated this revision to Diff 245261.Feb 18 2020, 2:05 PM

Delete the warning of .data2

It exposed another discrepancy which will be fixed by D74736

grimar added inline comments.Feb 19 2020, 1:30 AM
lld/ELF/LinkerScript.h
275

What about making this public and removing and inlining the warnChangedSectionAddresses code?

MaskRay updated this revision to Diff 245427.Feb 19 2020, 8:50 AM

Inline warnChangedSectionAddresses()

Thanks for clarifying, I don't have any more comments, happy if George is.

grimar accepted this revision.Feb 19 2020, 11:27 PM

LGTM

This revision is now accepted and ready to land.Feb 19 2020, 11:27 PM
This revision was automatically updated to reflect the committed changes.