[PATCH] D158961: [ThinLTO] Mark callers of local ifunc not eligible for import

[Fangrui Song via Phabricator via llvm-commits]

MaskRay created this revision.
MaskRay added reviewers: danielkiss, tavianator, tejohnson, yota9.
Herald added subscribers: hoy, ormris, steven.zhang, steven_wu, hiraditya, krytarowski, arichardson, inglorion, emaste.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Fix https://github.com/llvm/llvm-project/issues/58740
The `target_clones` attribute results in ifunc on eligible targets
(Linux glibc/Android or FreeBSD). If the function has internal linkage,
we will get an internal linkage ifunc.

  __attribute__((target_clones("popcnt", "default")))
  static int foo(int n) { return __builtin_popcount(n); }
  int use(int n) { return foo(n); }
  
  @foo.ifunc = internal ifunc i32 (i32), ptr @foo.resolver
  define internal nonnull ptr @foo.resolver() comdat {
  ; local linkage comdat is another issue that should be fixed
    ...
    select i1 %.not, ptr @foo.default.1, ptr @foo.popcnt.0
    ...
  }
  define internal i32 @foo.default.1(i32 noundef %n)

ifuncs are not included in module summaries, so LTO doesn't know the
local linkage `foo.default.1` referenced by `foo.resolver`
should be promoted. If a caller of `foo` (e.g. `use`) is imported,
the local linkage `foo.resolver` will be cloned as a definition
(IRLinker::shouldLink), leading to linker errors.

  ld.lld: error: undefined hidden symbol: foo.default.1.llvm.8017227050314953235
  >>> referenced by bar.c
  >>>               lto.tmp:(foo.ifunc)

As a simple fix, just mark `use` as not eligible for import. Non-local
linkage ifuncs do not have the problem, because they are not imported,
and not cloned when a caller is imported.

---

https://reviews.llvm.org/D82745 contains a more involved fix, though the
original bug it intended to fix
(https://github.com/llvm/llvm-project/issues/45833) now works.

Note: importing ifunc is tricky.
If we import an ifunc, we need to make sure the resolver and the
implementation are in the translation unit, as required by
https://sourceware.org/glibc/wiki/GNU_IFUNC

> Requirement (a): Resolver must be defined in the same translation unit as the implementations.

This is infeasible if the implementation is changed to
available_externally.

In addition, the imported ifunc may be referenced by two translation
units. This doesn't work with PowerPC32 -msecure-plt
(https://maskray.me/blog/2021-01-18-gnu-indirect-function).
At the very least, every referencing translation unit needs one extra
IRELATIVE dynamic relocation.

At least for the local linkage ifunc case, it doesn't have much use
outside of `target_clones`, as a global pointer is usually a better
replacement
(https://maskray.me/blog/2021-01-18-gnu-indirect-function#alternative).

I think ifuncs just have too many pitfalls to design more IR features
around it to optimize them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158961

Files:
  llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
  llvm/test/ThinLTO/X86/ifunc-import.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D158961.553827.patch
Type: text/x-patch
Size: 4641 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230828/e0e3a9f7/attachment.bin>