This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Refine LMA offset propagation rule in D76995
ClosedPublic

Authored by MaskRay on Jun 16 2020, 7:14 PM.

Details

Summary

If neither AT(lma) nor AT>lma_region is specified,
D76995 keeps lmaOffset (LMA - VMA) if the previous section is in the
default LMA region.

This patch additionally checks that the two sections are in the same
memory region.

Add a test case derived from https://bugs.llvm.org/show_bug.cgi?id=45313

.mdata : AT(0xfb01000) { *(.data); } > TCM
// It is odd to make .bss inherit lmaOffset, because the two sections
// are in different memory regions.
.bss : { *(.bss) } > DDR

With this patch, section VMA/LMA match GNU ld. Note, GNU ld supports
out-of-order (w.r.t sh_offset) sections and places .text and .bss in the
same PT_LOAD. We don't have that behavior.

Diff Detail

Event Timeline

MaskRay created this revision.Jun 16 2020, 7:14 PM

Sounds reasonable to me.

lld/ELF/LinkerScript.cpp
854

I know we did not do it in LLD before. But what do you think about starting using
the const for variables that are not suppored to change?

const bool sameMemRegion = ...
lld/test/ELF/linkerscript/lma-offset2.s
8

Perhaps a description about what this test case does wouldn't hurt.

17

I'd rename "DDR", "TCM" to something more clear like "TEXT", "DATA"

22

.mdata -> .data?

MaskRay updated this revision to Diff 271421.Jun 17 2020, 11:21 AM
MaskRay marked 4 inline comments as done.
MaskRay retitled this revision from [ELF] Refine LMA offset propagation rule in D76395 to [ELF] Refine LMA offset propagation rule in D76995.
MaskRay edited the summary of this revision. (Show Details)

Fix reference: it should be D76995

Add comments.

MaskRay added inline comments.Jun 17 2020, 11:23 AM
lld/ELF/LinkerScript.cpp
854

Good idea.

lld/test/ELF/linkerscript/lma-offset2.s
17

I don't know DDR but TCM but I think renaming to TEXT&DATA will make the test less reasonable.

.text and .bss are in the same memory region. .bss in TEXT will be weird.

22

I don't know .mdata but I think renaming it will make the test less reasonable.

It is weird for .data and .bss to be placed in different DATA memory regions.
.mdata is special and the distinction may be justified.

grimar accepted this revision.Jun 18 2020, 1:42 AM

LGTM

This revision is now accepted and ready to land.Jun 18 2020, 1:42 AM
This revision was automatically updated to reflect the committed changes.