Page MenuHomePhabricator

[ELF] Ignore the maximum of input section alignments for two cases
ClosedPublic

Authored by MaskRay on Feb 17 2020, 11:29 AM.

Details

Summary

Follow-up for D74286.

Notations:

  • alignExpr: the computed ALIGN value
  • max_input_align: the maximum of input section alignments

This patch changes the following two cases to match GNU ld:

  • When ALIGN is present, GNU ld sets output sh_addr to alignExpr, while lld use max(alignExpr, max_input_align)
  • When addrExpr is specified but alignExpr is not, GNU ld sets output sh_addr to addrExpr, while lld uses advance(0, max_input_align)

Note, sh_addralign is still set to max(alignExpr, max_input_align).

lma-align.test is enhanced a bit to check we don't overalign sh_addr.

fixSectionAlignments() sets addrExpr but not alignExpr for the !hasSectionsCommand case.
This patch sets alignExpr as well so that max_input_align will be respected.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 17 2020, 11:29 AM
MaskRay updated this revision to Diff 245042.Feb 17 2020, 3:12 PM

Adjust linkerscript/section-align2.test for a future change that adds warnings

grimar added inline comments.Feb 17 2020, 11:31 PM
lld/test/ELF/linkerscript/section-align2.test
2

Can it be x86?

27

The output section is aligned to 16.

But it is aligned to 32 in your output:

# CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
...
# CHECK-NEXT: .bss         NOBITS   0000000000020010 020009 000011 00  WA  0   0 32
grimar added inline comments.Feb 17 2020, 11:36 PM
lld/test/ELF/linkerscript/section-align2.test
2
MaskRay marked 4 inline comments as done.Feb 18 2020, 11:22 AM
MaskRay added inline comments.
lld/test/ELF/linkerscript/section-align2.test
2

aarch64 is to test thunks. AArch64 uses thunks, while x86 doesn't.

This helps us avoid https://sourceware.org/bugzilla/show_bug.cgi?id=25570

27

Its sh_addr is aligned to 16 though sh_addralign is 32 (max(output section alignment, maximum input section alignment))

This is GNU ld's behavior.

MaskRay updated this revision to Diff 245258.Feb 18 2020, 1:58 PM
MaskRay marked an inline comment as done.
MaskRay retitled this revision from [ELF] Use ALIGN to decide output section alignment to [ELF] Ignore the maximum of input section alignments for two cases.
MaskRay edited the summary of this revision. (Show Details)

Fix another discrepancy

MaskRay marked 2 inline comments as done.Feb 18 2020, 2:06 PM
MaskRay added inline comments.
lld/ELF/Writer.cpp
2242

I don't want to use early return to change the indentation of the large code block below.

lld/test/ELF/linkerscript/outsections-addr.s
86–88

I can do a follow-up change to change the llvm-readobj test to use llvm-readelf.

llvm-readobj -S output is very hard to read.

I agree with the specification of the change, I'm not sure why there is a change in Writer.cpp yet, if there is a good reason and it isn't obvious it will be worth a comment.

lld/ELF/Writer.cpp
2241

I'm struggling a bit with this one from the just the context. As far as I can tell there are only two uses of AlignExpr, in switchTo and in
void LinkerScript::adjustSectionsBeforeSorting()

`
    // Handle align (e.g. ".foo : ALIGN(16) { ... }").
    if (sec->alignExpr)
      sec->alignment =
          std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue());

In both cases I can't easily see why adding cmd->alignExpr = [align = cmd->alignment]() { return align; }; would change the calculated alignment. For example sec->alignment should be the same as alignExpr().getValue() and in switchTo

uint32_t align =
        sec->alignExpr ? sec->alignExpr().getValue() : ctx->outSec->alignment;

When there isn't alignExpr isn't ctx->outSec->alignment the same as alignExpr.getValue()?

Can you let me know if I'm missing something?

MaskRay marked an inline comment as done.Feb 19 2020, 10:43 AM
MaskRay added inline comments.
lld/ELF/Writer.cpp
2241

fixSectionAlignments() sets addrExpr but not alignExpr.

In LinkerScript::switchTo, the following branch applies:

if (sec->addrExpr && !sec->alignExpr) {
    // The alignment is ignored.
    ctx->outSec->addr = pos;
}

i.e. max_input_align is not respected.

We need it to take the else branch, thus set alignExpr as well.

MaskRay marked an inline comment as done.Feb 19 2020, 10:46 AM
MaskRay added inline comments.
lld/ELF/Writer.cpp
2241

In both cases I can't easily see why adding cmd->alignExpr = [align = cmd->alignment]() { return align; }; would change the calculated alignment.

cmd->alignExpr = [align = cmd->alignment]() { return align; }; makes LinkerScript::switchTo take the else branch.

sec->alignment = std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue()); sets sh_addralign, but the value is not used for deciding sh_addr.

psmith added inline comments.Feb 19 2020, 11:08 AM
lld/ELF/LinkerScript.cpp
765

Would it be worth a comment:
// When not using a linker script sec->alignExpr is set to the maximum of input section alignments.

lld/ELF/Writer.cpp
2241

Thanks for the clarification. That part seems easy to miss. I've put in a suggestion for a comment in switchTo.

MaskRay updated this revision to Diff 245478.Feb 19 2020, 11:22 AM
MaskRay marked 4 inline comments as done.

Improve a comment.

Thanks for the update, I'm happy if George is.

grimar added inline comments.Feb 20 2020, 12:54 AM
lld/ELF/LinkerScript.cpp
767

It is a bit strange to see that expandMemoryRegions is now gone from this path.
Is it correct?

lld/test/ELF/linkerscript/outsections-addr.s
86–88

I fully agree with "llvm-readobj -S output is very hard to read" here.

MaskRay marked an inline comment as done.Feb 20 2020, 7:28 AM
MaskRay added inline comments.
lld/ELF/LinkerScript.cpp
767

expandMemoryRegions(0) is a no-op.

grimar accepted this revision.Feb 20 2020, 11:31 PM

LGTM

This revision is now accepted and ready to land.Feb 20 2020, 11:31 PM
MaskRay updated this revision to Diff 245864.Feb 21 2020, 8:11 AM
MaskRay edited the summary of this revision. (Show Details)

Improve description

This revision was automatically updated to reflect the committed changes.

Hi, a bisect seems to show that this causes a kernel panic for an OOM test we have for zircon (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8887545931153261696/+/steps/failures/0/steps/Linux-_2_/0/steps/attempt_0__fail_/0/steps/failed:_host_x64_oom_tests/0/logs/stdio/0).

We're going to see if this is an us problem or a compiler problem, but just wanted to raise awareness in the meantime.

Hi, a bisect seems to show that this causes a kernel panic for an OOM test we have for zircon (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8887545931153261696/+/steps/failures/0/steps/Linux-_2_/0/steps/attempt_0__fail_/0/steps/failed:_host_x64_oom_tests/0/logs/stdio/0).

We're going to see if this is an us problem or a compiler problem, but just wanted to raise awareness in the meantime.

I'm 80% confident that this change is innocent, especially if zircon has never been tested with GNU ld...

It is highly recommended to write portable linker scripts, because lld/GNU ld's linker scripts have some subtle differences. lld's linker script support is catching up, but any improvement may unavoidably cause semantic differences.