[PATCH] D61613: [LLD][ELF] Add the -z ifunc-noplt option

[Mark Johnston via Phabricator via llvm-commits]

markj added a comment.

In D61613#1495998 <https://reviews.llvm.org/D61613#1495998>, @MaskRay wrote:

> Thanks for your explanation. I wanted to make sure the caveats of `-z ifunc-noplt` have been considered, the alternatives are compared before you decide to push forward these efforts :)


Certainly, I should have provided more justification in the review description. I appreciate the skepticism. :)

>> Over time, as vendors introduce new CPU features, our code accrues more and more of these tests. ifuncs are an attractive mitigation for this problem,
> 
> I will be glad if this helps some code organization problems! I did worry `-z ifunc-noplt` was proposed as it was a "fancy" feature. I asked musl guys, their feeling: "it's even worse on linux where they do this kind of hack; then happily make 10 abstraction layers; i don't get where folks get the idea to pour effort into wacky, costly microoptimizations like this when they have HUGE high-level performance problems". But they may not be aware of the FreeBSD situation.

I tend to agree with this view. FreeBSD is certainly not optimized to the point where a change like this is crucial. My two counter-points here would be:

- the patch is very small, and
- the patch is an optional optimization, i.e., if you decide to remove it in the future, FreeBSD will not lose any functionality.

If either of these statements were false, I would be much more reticent about trying to push the change upstream.

> [...]
> 
> In the description,
> 
>> Test Plan: I added a couple of regression tests and tested the FreeBSD kernel build using the latest lld sources.
> 
> If there are useful links, you may change this part of the commit description to include them.

Thanks for the feedback. I will update the review description to provide a better commit message.

In D61613#1496271 <https://reviews.llvm.org/D61613#1496271>, @ruiu wrote:

> Thank you for the explanation. I tend to oppose to add micro-optimizations to the ABI, but I agree that this particular optimization seems reasonable. I can imagine that with this feature you can use ifuncs in the kernel more liberally, which would make more difference in some test case.


Right, my concern is with ifunc usage over time. The kernel started using ifuncs late last year and now we are up to 51 ifuncs, and I'm sure the number will keep growing.



================
Comment at: lld/ELF/SyntheticSections.cpp:1238
   }
-  if (!Config->ZText)
+  if (!Config->ZText || Config->ZIfuncNoplt)
     DtFlags |= DF_TEXTREL;
----------------
MaskRay wrote:
> Oh, this should probably be restored.
> 
> In http://lists.llvm.org/pipermail/llvm-dev/2018-August/125490.html , I think your opinion was that `-z text -z ifunc-noplt` = error.
Ah, right, thank you. I added a check to readConfigs() instead and added a test to basic.s. Hopefully that's appropriate.


================
Comment at: lld/test/ELF/gnu-ifunc-noplt.s:38
+// DISASM-NEXT:   201026:       ff 25 e4 1f 00 00       jmpq    *8164(%rip)
+// DISASM-NEXT:   20102c:       0f 1f 40 00     nopl    (%rax)
+// DISASM-EMPTY:
----------------
MaskRay wrote:
> markj wrote:
> > MaskRay wrote:
> > > You FreeBSD llvm-objdump is old XD
> > > 
> > > The misindentation made me unhappy so I fixed it in rL358405.
> > Not too old :)
> > 
> > ```
> > > llvm-objdump --version
> > LLVM (http://llvm.org/):
> >   LLVM version 8.0.0
> > ```
> My change will be included by llvm 9.0.0 ;-) Since you've cloned the monorepo, you may run `ninja -C your/build llvm-objdump` and use that llvm-objdump to update the tests.
> 
> I still suggest fixing the misaligned instructions before you commit...
Ok, but I believe I fixed the misalignment before your last comment here? I can't see any others.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61613/new/

https://reviews.llvm.org/D61613