Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"ifunc must point to a defined function" error for static IFUNC resolver functions in C++ mode #54549

Closed
BertalanD opened this issue Mar 25, 2022 · 9 comments

Comments

@BertalanD
Copy link
Member

BertalanD commented Mar 25, 2022

The following code compiles correctly (using both Clang 13.0.1 and trunk) in C mode, but returns an "ifunc must point to a defined function" error when compiled with clang++.

#ifdef __cplusplus
extern "C" {
#endif

__attribute__((used)) static void* resolve_foo() {
    return 0;
}

__attribute__((ifunc("resolve_foo"))) void foo();
#ifdef __cplusplus
}
#endif

Removing the static storage class specifier makes it compile in C++ mode too.

The documentation explicitly states that IFUNC resolver functions may be static, so I think this is a bug.

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Mar 25, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 25, 2022

@llvm/issue-subscribers-clang-frontend

@erichkeane
Copy link
Collaborator

I was able to repro this with:

static void *resolve_foo() { return 0;}
__attribute__((ifunc("resolve_foo"))) void foo();

clang -cc1 temp.cpp -emit-llvm (emit-llvm necessary because this diagnostic comes from the Clang CodeGen).

@BertalanD
Copy link
Member Author

@erichkeane Please note that your reduced example doesn't build with gcc either, as the parameter of the ifunc attribute needs to be the mangled name of the resolver (so should be _ZL11resolve_foov instead).

This is why I added extern "C" in the first place.

@erichkeane
Copy link
Collaborator

Ah! Interesting, thank you for the clarification! I note that the 'IR' when you do extern "C" around a static function is to cause it to create an 'alias' to the mangled name. At the point we are checking it though, we don't have that alias created yet however, so we just see an undefined function.

For anyone who sees this later, CodeGenModule's checkAliases sees:


@foo = ifunc void (), void ()* ()* @resolve_foo

; Function Attrs: mustprogress noinline nounwind optnone
define internal noundef i8* @_ZL11resolve_foov() #0 {
entry:
  ret i8* null
}

declare void ()* @resolve_foo()

BUT the final IR for resolve_foo ends up as:

@resolve_foo = internal alias i8* (), i8* ()* @_ZL11resolve_foov

define internal noundef i8* @_ZL11resolve_foov() #0 !dbg !8 {
entry:
  ret i8* null, !dbg !14
}

Note that when checking, the 'resolver' is just a function declaration, and not yet the extern "C" caused alias.

@erichkeane
Copy link
Collaborator

I was able to find that the alias for extern "C" gets created in EmitStaticExternCAliases. in CodeGenModule::Release, this gets called significantly after we checkAliases (and in fact, after we do a LOT of other things!). I tried moving it after (so that the CheckAliases), BUT the existence of the resolver-created 'function declaration' of the same name suppresses creation of the alias in EmitStaticExternCAliases. I tried letting it create the alias anyway, but replaceAllUsesWith ends up not working because the types end up changing (Function to alias).

I suspect there is a bit of work that needs to happen to make sure we update for this particular case, but I'm not sure
a- if this ends up breaking a bunch of other stuff
b- if the ifunc diagnostics are smart enough to follow aliases.

I'm out of time for the day, so if I or others come across this, perhaps this can save some time. Else, if I have time to, I'll take a look again on Monday.

@erichkeane
Copy link
Collaborator

The patch to fix this was getting complicated, so I ended up having a question on the mailing list: https://discourse.llvm.org/t/why-are-extern-c-static-functions-emitted-with-c-mangling/61268

Depending on the answer to that, I can have my implementation unblocked.

@erichkeane
Copy link
Collaborator

Review here: https://reviews.llvm.org/D122608

@EugeneZelenko EugeneZelenko added clang:codegen and removed clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 28, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 28, 2022

@llvm/issue-subscribers-clang-codegen

erichkeane pushed a commit that referenced this issue Apr 1, 2022
We expect that `extern "C"` static functions to be usable in things like
inline assembly, as well as ifuncs:
See the bug report here: #54549

However, we were diagnosing this as 'not defined', because the
ifunc's attempt to look up its resolver would generate a declared IR
function.

Additionally, as background, the way we allow these static extern "C"
functions to work in inline assembly is by making an alias with the C
mangling in MOST situations to the version we emit with
internal-linkage/mangling.

The problem here was multi-fold: First- We generated the alias after the
ifunc was checked, so the function by that name didn't exist yet.
Second, the ifunc's generation caused a symbol to exist under the name
of the alias already (the declared function above), which suppressed the
alias generation.

This patch fixes all of this by moving the checking of ifuncs/CFE aliases
until AFTER we have generated the extern-C alias.  Then, it does a
'fixup' around the GlobalIFunc to make sure we correct the reference.

Differential Revision: https://reviews.llvm.org/D122608
@erichkeane
Copy link
Collaborator

Fixed! 9ba8c40

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
We expect that `extern "C"` static functions to be usable in things like
inline assembly, as well as ifuncs:
See the bug report here: llvm/llvm-project#54549

However, we were diagnosing this as 'not defined', because the
ifunc's attempt to look up its resolver would generate a declared IR
function.

Additionally, as background, the way we allow these static extern "C"
functions to work in inline assembly is by making an alias with the C
mangling in MOST situations to the version we emit with
internal-linkage/mangling.

The problem here was multi-fold: First- We generated the alias after the
ifunc was checked, so the function by that name didn't exist yet.
Second, the ifunc's generation caused a symbol to exist under the name
of the alias already (the declared function above), which suppressed the
alias generation.

This patch fixes all of this by moving the checking of ifuncs/CFE aliases
until AFTER we have generated the extern-C alias.  Then, it does a
'fixup' around the GlobalIFunc to make sure we correct the reference.

Differential Revision: https://reviews.llvm.org/D122608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants