This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support linking ET_EXEC
AbandonedPublic

Authored by MaskRay on Mar 13 2020, 11:27 PM.

Details

Summary

GNU ld supports linking an ET_EXEC as if it were an ET_REL. This is an
extremely rare feature, but we can implement it with just one more line.

As a real world use case, the final link phase of seabios does:

ld -N -T out/romlayout32flat.lds out/rom16.strip.o out/rom32seg.strip.o out/code32flat.o -o out/rom.o

out/rom16.strip.o and out/rom32seg.strip.o are ET_EXEC created by previous steps.

romlayout32flat.lds collects the pieces and produces rom.o which
contains code of all x86 memory models.

With this lld patch and two seabios patches, I manage to link seabios with lld.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 13 2020, 11:27 PM
grimar accepted this revision.Mar 14 2020, 7:29 AM

I think a one-line patch even for such a rare feature is OK, so LGTM. But lets wait for a second opinion too.

lld/ELF/Driver.cpp
262

I wonder if we want a comment here.

lld/test/ELF/executable.test
19

Nit: this could be a single line:

# RUN: echo "SECTIONS { .foo : { *(.text) } }" > %t.script

This test file could be *.s.

This revision is now accepted and ready to land.Mar 14 2020, 7:29 AM
MaskRay updated this revision to Diff 250390.Mar 14 2020, 3:17 PM

Add a comment

MaskRay updated this revision to Diff 250391.Mar 14 2020, 3:21 PM

Rename executable.test to executable.s and switch the role of assembly/linker script

Harbormaster completed remote builds in B49233: Diff 250391.

I can see that this would work if the user were very careful and as it is a legitimate use case we should support it. In a previous linker we were asked to make this case an error as a customer had done this by mistake and spent several hours wondering why their program wasn't working. On balance I think it isn't worth a warning along the lines of "warning: file X is an executable" as I don't think that this happens much in practice, but something to think about if other users have problems.

Looks good to me.

MaskRay abandoned this revision.Mar 15 2020, 6:49 PM

I can see that this would work if the user were very careful and as it is a legitimate use case we should support it. In a previous linker we were asked to make this case an error as a customer had done this by mistake and spent several hours wondering why their program wasn't working. On balance I think it isn't worth a warning along the lines of "warning: file X is an executable" as I don't think that this happens much in practice, but something to think about if other users have problems.

Looks good to me.

Thanks for the feedback. On second thought, I think I shall abandon the patch. This is easy to workaround on seabios' side

https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/HHIUPUXRIZ3KLTK4TPLG2V4PFP32HRBE/

If seabios accepts my 3 patches, they can link seabios with lld 7. Projects like FreeBSD 12 (lld 8.0.1) can immediately benefit from it.

lld/test/ELF/executable.test