Preserve relocations against ifuncs when -zifunc-noplt is specified.

[markj]

Summary

• Modify isStaticLinkTimeConstant() so that lld is required to pass ifunc call relocations through the static link phase.
• Ensure that ifunc symbols are added to the dynamic symbol table, since we need to be able to refer to them from dynamic relocations.
• Add relocations against ifunc symbols to .rel(a).dyn.

I propose committing this and accompanying kernel linker changes
for 12.0. Once we are less pressed for time I plan to try upstreaming
this change. Since the patch is small I believe it won't be much of a
maintenance burden.

Test Plan

I used the new flag in the i386 and amd64 kernel builds, see
D16749 and D16750. With this optimization ifunc calls look
the same as any others:

(kgdb) disas fuword
Dump of assembler code for function fuword:
0xffffffff80799b50 <fuword+0>:  push   %rbp
0xffffffff80799b51 <fuword+1>:  mov    %rsp,%rbp
0xffffffff80799b54 <fuword+4>:  sub    $0x10,%rsp
0xffffffff80799b58 <fuword+8>:  lea    -0x8(%rbp),%rsi
0xffffffff80799b5c <fuword+12>: callq  0xffffffff80adc4a0 <fueword_nosmap>
0xffffffff80799b61 <fuword+17>: mov    %eax,%ecx
0xffffffff80799b63 <fuword+19>: mov    $0xffffffffffffffff,%rax
0xffffffff80799b6a <fuword+26>: cmp    $0xffffffffffffffff,%ecx
0xffffffff80799b6d <fuword+29>: je     0xffffffff80799b73 <fuword+35>
0xffffffff80799b6f <fuword+31>: mov    -0x8(%rbp),%rax
0xffffffff80799b73 <fuword+35>: add    $0x10,%rsp
0xffffffff80799b77 <fuword+39>: pop    %rbp
0xffffffff80799b78 <fuword+40>: retq

[emaste]

I think this is good and agree it should be easy to carry as a local patch while coordinating upstreaming. Man page update needed too though.

[markj]

• update ld.lld.1

[arichardson]

LGTM

contrib/llvm/tools/lld/ELF/Relocations.cpp

1049 ↗

(On Diff #46783)

I added an overload of addReloc to lld that should remove the need for this if/else: void addReloc(RelType DynType, InputSectionBase *InputSec, uint64_t OffsetInSec, Symbol *Sym, int64_t Addend, RelExpr Expr, RelType Type); but it might only have got in for 7.0.

[markj]

contrib/llvm/tools/lld/ELF/Relocations.cpp

1049 ↗

(On Diff #46783)

Indeed, it looks like this code's been modified a fair bit in 7. In the next couple of days I'll try to port the patch to @dim's llvm 7 branch.

[emaste]

contrib/llvm/tools/lld/ELF/Relocations.cpp

1049 ↗

(On Diff #46783)

Yeah, I'd suggest we:

1. commit this to head
2. update to apply to lld 7 and apply to the clang700-import branch
3. propose upstream

[dim]

I haven't yet had time to look at this, but it should really be suggested and vetted upstream first, before applying it locally.

With the previous patch, I still haven't been able to apply it upstream, due to missing and failing tests, for example.

[markj]

In D16748#356477, @dim wrote:

I haven't yet had time to look at this, but it should really be suggested and vetted upstream first, before applying it locally.

Ok. I'll port the patch to 7 today and post it to a mailing list. Do you want to see the patch committed upstream before landing it in FreeBSD?

With the previous patch, I still haven't been able to apply it upstream, due to missing and failing tests, for example.

Do you mean r337282? I'll work on getting that upstreamed together with this patch. Just been busy with 12.0 stuff.

[dim]

Oh sorry, I didn't realize that this is needed pretty quickly due to the slush, so then it is probably okay. I'd need some time to look at it and build it for testing, but for now I'll assume you've tested it and it works. :)

Regarding rS337282, I submitted that upstream already, in https://reviews.llvm.org/D50297, but it turned out it broke a lot of tests. This is due to the fact that it changes the base addresses for all operating systems, but it looks like upstream is OK with that. (It would be more work to add OS-specific handling for it.) What needs to be done is to go over all the changed test cases, and update them, making sure the output is as expected.

[markj]

In D16748#356731, @dim wrote:

Oh sorry, I didn't realize that this is needed pretty quickly due to the slush, so then it is probably okay. I'd need some time to look at it and build it for testing, but for now I'll assume you've tested it and it works. :)

Indeed, I've been testing it on amd64 and i386 while working on the associated kernel changes in D16749. I'll port it to your llvm 7 branch now.

Regarding rS337282, I submitted that upstream already, in https://reviews.llvm.org/D50297, but it turned out it broke a lot of tests. This is due to the fact that it changes the base addresses for all operating systems, but it looks like upstream is OK with that. (It would be more work to add OS-specific handling for it.) What needs to be done is to go over all the changed test cases, and update them, making sure the output is as expected.

Oh, thank you! Do you plan on updating the tests yourself?

[markj]

I asked for comments from upstream here: http://lists.llvm.org/pipermail/llvm-dev/2018-August/125460.html

As long no glaring problems are pointed out, I'd like to commit this in the next couple of days.

[emaste]

usr.bin/clang/lld/ld.lld.1

450 ↗

(On Diff #46788)

As Peter Smith suggested in the thread we should add a warning/caveat here that this option requires support from the rtld or self-relocation code.

[markj]

• update man page

[markj]

I plan to commit this in the next few hours if there are no objections. I've ported the patch to the clang700-import branch here: https://github.com/markjdb/freebsd-dev/commit/02f35faa6df364769b9223746b99e3c7ba05c5dd

I'm currently unable to test it with i386 since lld currently emits the writeable linker sets such that the locore.s .bss zeroing code overwrites those sections in addition to .bss.