[ELF] Relocations: amends c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0

[adalava]

Summary

It's expected to set set hasDirectReloc only if not ifunc.

This fixes the issue described in https://reviews.freebsd.org/D36234

[MaskRay]

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

[adalava]

In D133745#3785479, @MaskRay wrote:

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

@MaskRay, I don't understand, the commit c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0 message says that it will "set hasDirectReloc only if not ifunc", but the change seems to do the opposite, that's why I'm proposing the amend change (and because it fixes the FreeBSD copyin/copyout symbol).

The issue I'm pursuing started after https://reviews.freebsd.org/rG47a57144af25a7bd768b29272d50a36fdf2874ba, where the IFUNC copyin/copyout is called thought a struct of function pointers. In LLD I found that the symbols get HAS_DIRECT_RELOC set and the type is changed from STT_GNU_IFUNC to STT_FUNC, then kernel modules crash when try to call them. It can be workarounded by creating a local wrapper function that just calls the function intended (code in https://reviews.freebsd.org/D36234). Do you have an idea of what else is missing on LLD?

I had no success creating an small code that reproduces it, but I can share the lld's reproducer for the kernel. Thanks!

[MaskRay]

In D133745#3785538, @adalava wrote:

In D133745#3785479, @MaskRay wrote:

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

@MaskRay, I don't understand, the commit c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0 message says that it will "set hasDirectReloc only if not ifunc", but the change seems to do the opposite, that's why I'm proposing the amend change (and because it fixes the FreeBSD copyin/copyout symbol).

I made a mistake in the commit message. It should be "if ifunc"... You can verify that hasDirectReloc (now HAS_DIRECT_RELOC) is only functional when isGnuIfunc() is true.

The issue I'm pursuing started after https://reviews.freebsd.org/rG47a57144af25a7bd768b29272d50a36fdf2874ba, where the IFUNC copyin/copyout is called thought a struct of function pointers. In LLD I found that the symbols get HAS_DIRECT_RELOC set and the type is changed from STT_GNU_IFUNC to STT_FUNC, then kernel modules crash when try to call them. It can be workarounded by creating a local wrapper function that just calls the function intended (code in https://reviews.freebsd.org/D36234). Do you have an idea of what else is missing on LLD?

I had no success creating an small code that reproduces it, but I can share the lld's reproducer for the kernel. Thanks!

I need a reproduce tarball to analyze. shared STT_GNU_IFUNC becomes STT_FUNC in Symbols.h:436 and an ifunc with HAS_DIRECT_RELOC is converted to STT_FUNC, too.
The changes are necessary for correct semantics.

[adalava]

In D133745#3785544, @MaskRay wrote:

I made a mistake in the commit message. It should be "if ifunc"... You can verify that hasDirectReloc (now HAS_DIRECT_RELOC) is only functional when isGnuIfunc() is true.

oh, np!

I need a reproduce tarball to analyze. shared STT_GNU_IFUNC becomes STT_FUNC in Symbols.h:436 and an ifunc with HAS_DIRECT_RELOC is converted to STT_FUNC, too.
The changes are necessary for correct semantics.

Here it is, with more details: https://github.com/llvm/llvm-project/issues/57722

Thanks!

lld/ELF/Relocations.cpp

 //===- Relocations.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (config->emachine == EM_MIPS) {
       // for detailed description:
       // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
       in.mipsGot->addEntry(*sec->file, sym, addend, expr);
     } else {
       sym.setFlags(NEEDS_GOT);
     }
   } else if (needsPlt(expr)) {
     sym.setFlags(NEEDS_PLT);
-  } else if (LLVM_UNLIKELY(isIfunc)) {
+  } else if (LLVM_UNLIKELY(!isIfunc)) {
     sym.setFlags(HAS_DIRECT_RELOC);
   }
 
   processAux(expr, type, offset, sym, addend);
 }
 
 // R_PPC64_TLSGD/R_PPC64_TLSLD is required to mark `bl __tls_get_addr` for
 // General Dynamic/Local Dynamic code sequences. If a GD/LD GOT relocation is