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

[clang] Reporting unused function with ifunc resolver #63957

Closed
JiaT75 opened this issue Jul 19, 2023 · 9 comments · Fixed by #87130
Closed

[clang] Reporting unused function with ifunc resolver #63957

JiaT75 opened this issue Jul 19, 2023 · 9 comments · Fixed by #87130
Assignees
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer

Comments

@JiaT75
Copy link

JiaT75 commented Jul 19, 2023

Hello!

Clang 16.0.0 and older report a warning when compiling a program that uses __attribute__((__ifunc__())) with -Wunused-function. The resolver function appears to trigger this when it is only referenced by the __attribute__. Here is an example program that should trigger the warning:

static void resolved(void) {
    return;
}

static void (*resolver (void)) (void) {
	return resolved;
}

void indirect_function(void) __attribute__((__ifunc__("resolver")));

int main() {
    indirect_function();
}

GCC does not issue a warning for the above program. Here is a godbolt.org snippet of the above code to allow you to easily test it. Let me know if you need anymore information.

@EugeneZelenko EugeneZelenko added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed new issue labels Jul 19, 2023
@Zentrik
Copy link
Contributor

Zentrik commented Mar 29, 2024

This reproduces on trunk.

Zentrik added a commit to Zentrik/julia that referenced this issue Mar 29, 2024
…and ifunc support

Clang incorrectly warns functions only used through ifuncs are unused, llvm/llvm-project#63957.
@mxmlnkn
Copy link

mxmlnkn commented Mar 29, 2024

Should this really be changed taking into account the ifunc-based xz backdoor added by the issue reporter? https://www.openwall.com/lists/oss-security/2024/03/29/4

@tstellar
Copy link
Collaborator

We should not make this change in LLVM right now.

@MaskRay
Copy link
Member

MaskRay commented Mar 30, 2024

We should not make this change in LLVM right now.

tukaani-project/xz@82ecc53 is a legitimate change that exposes two problems:

We should be careful and don't consider all xz commits malicious.

@tstellar
Copy link
Collaborator

We should be careful and don't consider all xz commits malicious.

I disagree, we need to be extra careful with changes associated with the xz project right now. There's no reason why we need to get this change committed quickly. It should get extra scrutiny.

@shafik
Copy link
Collaborator

shafik commented Mar 30, 2024

We should be careful and don't consider all xz commits malicious.

I disagree, we need to be extra careful with changes associated with the xz project right now. There's no reason why we need to get this change committed quickly. It should get extra scrutiny.

I concur with this sentiment, the details are still emerging on this whole incident. This so far seems like the best timeline: https://boehs.org/node/everything-i-know-about-the-xz-backdoor and a lot if still unknown.

I don't feel confident in saying I have the whole picture and I think until we get a better idea of the scope we should be taking our time.

@NuLL3rr0r
Copy link

Did GitHub just restored their access? I thought the account was suspended.

@vvdaal
Copy link

vvdaal commented Mar 31, 2024

Rather offtopic but

Did GitHub just restored their access? I thought the account was suspended.

No (check https://github.com/Larhzu?tab=following), Larhzu is also still suspended. (https://github.com/JiaT75?tab=following)

@llvm llvm locked as off-topic and limited conversation to collaborators Mar 31, 2024
@thesamesam
Copy link
Member

I'm going to lock this for now but we can unlock it later when things have quietened down.

@llvm llvm deleted a comment from ItzSwirlz Apr 1, 2024
MaskRay added a commit that referenced this issue Apr 16, 2024
https://reviews.llvm.org/D54188 marked "alias" targets as used in C to
fix -Wunused false positives. This patch extends the approach to handle
mangled names to support global scope names in C++ and the
`overloadable` attribute in C.

In addition, we mark ifunc targets as used to fix #63957.

While our approach has false negatives for namespace scope names, the
majority of alias/ifunc C++ uses (global scope with no overloads) are
handled.

Note: The following function with internal linkage but C language
linkage type is mangled in Clang but not in GCC. This inconsistency
makes alias/ifunc difficult to use in C++ with portability (#88593).
```
extern "C" {
static void f0() {}
// GCC: void g0() __attribute__((alias("_ZL2f0v")));
// Clang: void g0() __attribute__((alias("f0")));
}
```

Pull Request: #87130
MaskRay added a commit that referenced this issue Apr 16, 2024
https://reviews.llvm.org/D54188 marked "alias" targets as used in C to
fix -Wunused false positives. This patch extends the approach to handle
mangled names to support global scope names in C++ and the
`overloadable` attribute in C.

(Note: we should skip `UsingShadowDecl`, which would trigger an
assertion failure in `ItaniumMangleContextImpl::mangleCXXName`.
See regression test added by commit 1c2afba.)

In addition, we mark ifunc targets as used to fix #63957 (temporarily
used by xz; ifunc was removed by
tukaani-project/xz@689ae24)

While our approach has false negatives for namespace scope names, the
majority of alias/ifunc C++ uses (global scope with no overloads) are
handled.

Note: The following function with internal linkage but C language
linkage type is mangled in Clang but not in GCC. This inconsistency
makes alias/ifunc difficult to use in C++ with portability (#88593).
```
extern "C" {
static void f0() {}
// GCC: void g0() __attribute__((alias("_ZL2f0v")));
// Clang: void g0() __attribute__((alias("f0")));
}
```

Pull Request: #87130
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants