This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Implement option to force PIC compatible Thunks
ClosedPublic

Authored by peter.smith on Dec 10 2018, 4:13 AM.

Details

Summary

By default LLD will generate position independent Thunks when the --pie or --shared option is used. Reference to absolute addresses is permitted in other cases. For some embedded systems position independent thunks are needed for code that executes before the MMU has been set up. The option --pic-veneer is used by ld.bfd to force position independent thunks.

The patch adds an option --pic-thunk with --pic-veneer as an alias to support this option. The --pic-veneer option is needed for the Linux kernel on Arm.

Fixes pr39886 (https://bugs.llvm.org/show_bug.cgi?id=39886)

Diff Detail

Event Timeline

peter.smith created this revision.Dec 10 2018, 4:13 AM

You need to add the new option(s) to lld\docs\ld.lld.1 I think.

ELF/Driver.cpp
821

Should it have Config->Pic argument as a default (and a OPT_no_pic_thunk)?

Config->PicThunk = Args.hasArg(OPT_pic_thunk, OPT_no_pic_thunk, Config->Pic);
ELF/Thunks.cpp
722

Then you could remove this helper.

test/ELF/arm-force-pi-thunk.s
13

I would add empty lines before/after this comment.

Thanks very much for the comments, I've applied the suggested changes and updated the docs.

The code now looks good to me, thanks!
(I do not know much about ARM and thunks to give a sign off I think)

tpimh added a subscriber: tpimh.Dec 10 2018, 10:49 AM

In my test case using Linux compiled for Arm using allyesconfig and ld.lld with these changes applied linking fails as follows:

...
ld.lld: error: relocation R_ARM_MOVT_PREL out of range: -4403772844 is not in [-2147483648, 2147483647]
...

I guess this is since we are using R_ARM_MOVT_PREL in ARMV7PILongThunk. Not sure whether we really care as this is more a constructed case with an insanely big kernel (with ld.bfd the linked kernel ends up at 201MiB!).

ld.bfd seems to use a different approach which allows the kernel to successfully link. See also:
https://lore.kernel.org/patchwork/patch/1021192/#1213770

In my test case using Linux compiled for Arm using allyesconfig and ld.lld with these changes applied linking fails as follows:

...
ld.lld: error: relocation R_ARM_MOVT_PREL out of range: -4403772844 is not in [-2147483648, 2147483647]
...

I guess this is since we are using R_ARM_MOVT_PREL in ARMV7PILongThunk. Not sure whether we really care as this is more a constructed case with an insanely big kernel (with ld.bfd the linked kernel ends up at 201MiB!).

ld.bfd seems to use a different approach which allows the kernel to successfully link. See also:
https://lore.kernel.org/patchwork/patch/1021192/#1213770

I'll address the MOVT/MOVW relocation overflow when I get back to the office on Monday. At a first pass I think the overflow calculation for MOVT/MOVW isn't permissive enough, at least when used from the thunks. In any case I'll use a separate patch.

ruiu added a comment.Jan 2 2019, 11:35 AM

Generally looking good.

What is a reason to add the option as --pic-thunk with an alias of --pic-veneer? If existing code uses --pic-veneer, maybe we could just add that option?

Generally looking good.

What is a reason to add the option as --pic-thunk with an alias of --pic-veneer? If existing code uses --pic-veneer, maybe we could just add that option?

It was only to be consistent with LLD's use of thunk. I'll update the patch with just --pic-veneer as the command line option to select it.

Updated to remove --pic-thunk command line option. We now use only the ld.bfd option --pic-veneer. No other changes.

I've created D56396 to cover the relocation out of range errors seen when linking the linux kernel.

Ping. I think that all the currently requested changes have been made and the out of range link errors problem caused by enabling it has been fixed in D56396.

ruiu accepted this revision.Jan 15 2019, 11:16 AM

LGTM

This revision is now accepted and ready to land.Jan 15 2019, 11:16 AM
This revision was automatically updated to reflect the committed changes.