Skip to content

[Sema] Mark alias/ifunc targets used and consider mangled names #87130

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 16, 2024
Merged

[Sema] Mark alias/ifunc targets used and consider mangled names #87130

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
23422a0
[𝘀𝗽𝗿] initial version
MaskRay Mar 30, 2024
9ae8958
improve test
MaskRay Apr 2, 2024
3bd4297
avoid false negatives
MaskRay Apr 4, 2024
6f2a3da
code style
MaskRay Apr 4, 2024
1b3847e
use -triple %itanium_abi_triple
MaskRay Apr 4, 2024
c3793ad
improve tests. use FreeDeleter
MaskRay Apr 5, 2024
1e8d74c
rebase after test updates
MaskRay Apr 12, 2024
fb2b988
use x86_64-linux since Darwin doesn't support ifunc
MaskRay Apr 13, 2024
5b85c1b
refer to #88825
MaskRay Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/lib/Sema/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
set(LLVM_LINK_COMPONENTS
Core
Demangle
FrontendHLSL
FrontendOpenMP
MC
Expand Down
44 changes: 33 additions & 11 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include "clang/Sema/SemaInternal.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Demangle/Demangle.h"
#include "llvm/IR/Assumptions.h"
#include "llvm/MC/MCSectionMachO.h"
#include "llvm/Support/Error.h"
Expand Down Expand Up @@ -1980,6 +1981,36 @@ static void handleWeakRefAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
D->addAttr(::new (S.Context) WeakRefAttr(S.Context, AL));
}

// Mark alias/ifunc target as used. Due to name mangling, we look up the
// demangled name ignoring parameters. This should handle the majority of use
// cases while leaving namespace scope names unmarked.
static void markUsedForAliasOrIfunc(Sema &S, Decl *D, const ParsedAttr &AL,
StringRef Str) {
char *Demangled = nullptr;
if (S.getASTContext().getCXXABIKind() != TargetCXXABI::Microsoft)
Demangled = llvm::itaniumDemangle(Str, /*ParseParams=*/false);
std::unique_ptr<MangleContext> MC(S.Context.createMangleContext());
SmallString<256> Name;

const DeclarationNameInfo Target(
&S.Context.Idents.get(Demangled ? Demangled : Str), AL.getLoc());
LookupResult LR(S, Target, Sema::LookupOrdinaryName);
if (S.LookupName(LR, S.TUScope)) {
for (NamedDecl *ND : LR) {
if (MC->shouldMangleDeclName(ND)) {
llvm::raw_svector_ostream Out(Name);
Name.clear();
MC->mangleName(GlobalDecl(ND), Out);
} else {
Name = ND->getIdentifier()->getName();
}
if (Name == Str)
ND->markUsed(S.Context);
}
}
free(Demangled);
}

static void handleIFuncAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
StringRef Str;
if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
Expand All @@ -1992,6 +2023,7 @@ static void handleIFuncAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
return;
}

markUsedForAliasOrIfunc(S, D, AL, Str);
D->addAttr(::new (S.Context) IFuncAttr(S.Context, AL, Str));
}

Expand Down Expand Up @@ -2026,17 +2058,7 @@ static void handleAliasAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
}
}

// Mark target used to prevent unneeded-internal-declaration warnings.
if (!S.LangOpts.CPlusPlus) {
// FIXME: demangle Str for C++, as the attribute refers to the mangled
// linkage name, not the pre-mangled identifier.
const DeclarationNameInfo target(&S.Context.Idents.get(Str), AL.getLoc());
LookupResult LR(S, target, Sema::LookupOrdinaryName);
if (S.LookupQualifiedName(LR, S.getCurLexicalContext()))
for (NamedDecl *ND : LR)
ND->markUsed(S.Context);
}

markUsedForAliasOrIfunc(S, D, AL, Str);
D->addAttr(::new (S.Context) AliasAttr(S.Context, AL, Str));
}

Expand Down
1 change: 1 addition & 0 deletions clang/test/AST/ast-dump-attr-json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ __thread __attribute__ ((tls_model ("local-exec"))) int tls_model_var;
// CHECK-NEXT: "tokLen": 11
// CHECK-NEXT: }
// CHECK-NEXT: },
// CHECK-NEXT: "isUsed": true,
// CHECK-NEXT: "name": "global_decl",
// CHECK-NEXT: "mangledName": "global_decl",
// CHECK-NEXT: "type": {
Expand Down
7 changes: 0 additions & 7 deletions clang/test/Sema/alias-unused.c
View file
Open in desktop

This file was deleted.

47 changes: 47 additions & 0 deletions clang/test/Sema/alias-unused.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// RUN: %clang_cc1 -triple %itanium_abi_triple -Wunused -x c -verify %s
// RUN: %clang_cc1 -triple %itanium_abi_triple -Wunused -x c++ -verify=expected,cxx %s

#ifdef __cplusplus
extern "C" {
#endif
static int f(void) { return 42; }
int g(void) __attribute__((alias("f")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me, a false negative.

FWICT, for C++ mode, even in an extern "C" block, it seems that functions with static linkage get mangled. Or... Oh, look, more behavioral differences from GCC: https://godbolt.org/z/sa4e7aYqj

We mangle, they do not. That's going to lead to further compatibility issues when trying to use this function attribute.

Ah! right you pointed this out in your note. Perhaps we should fix/change that first in clang?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! right you pointed this out in your note. Perhaps we should fix/change that first in clang?

I remember an existing issue about the mangling difference, but I don't recall the link now.
I have analyzed it a bit and run into a feature where we need mangling.
The discrepancy from GCC is indeed unfortunate and in practice makes alias/ifunc not useful for internal linkage targets in C++.

https://eel.is/c++draft/dcl.link gives an example

extern "C" {
  static void f4();             // the name of the function f4 has internal linkage,
                                // so f4 has no language linkage; its type has C language linkage
}

I am unsure whether "no language linkage" means that ignoring the C language linkage is fine.

Improving GCC consistency is not the goal of this PR. This PR intends to make clangSema and clangCodeGen consistent. If we want to follow GCC in the future, the updated test will show the differences.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improving GCC consistency is not the goal of this PR. This PR intends to make clangSema and clangCodeGen consistent.

Is there a bug on file wrt to this inconsistency? I would like to link to it from the test, sources, and public docs on the function attribute if we don't fix this inconsistency BEFORE landing this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a bug and link to it from a TODO comment in the tests and/or sources.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #88593

But I don't think a TODO applies. Clang's behavior is probably desired. So this just records a difference.

This difference probably doesn't make a difference because people rarely use alias for C++ code.


static int foo [] = { 42, 0xDEAD }; // cxx-warning{{variable 'foo' is not needed and will not be emitted}}
extern typeof(foo) bar __attribute__((unused, alias("foo")));

static int (*resolver(void))(void) { return f; } // cxx-warning{{unused function 'resolver'}}
int ifunc(void) __attribute__((ifunc("resolver")));

static int __attribute__((overloadable)) f0(int x) { return x; }
static float __attribute__((overloadable)) f0(float x) { return x; } // expected-warning{{unused function 'f0'}}
int g0(void) __attribute__((alias("_ZL2f0i")));

#ifdef __cplusplus
static int f1() { return 42; }
int g1(void) __attribute__((alias("_ZL2f1v")));
}

/// We demangle alias/ifunc target and mark all found functions as used.

static int f2(int) { return 42; } // cxx-warning{{unused function 'f2'}}
static int f2() { return 42; }
int g2() __attribute__((alias("_ZL2f2v")));

static int (*resolver1())() { return f; } // cxx-warning{{unused function 'resolver1'}}
static int (*resolver1(int))() { return f; }
int ifunc1() __attribute__((ifunc("_ZL9resolver1i")));

/// We should report "unused function" for f3(int).
namespace ns {
static int f3(int) { return 42; } // cxx-warning{{unused function 'f3'}}
static int f3() { return 42; } // cxx-warning{{unused function 'f3'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While our approach has false negatives for namespace scope names

Isn't this with ns::f3() here a false positive, not a false negative? Consider adding TODO: to make it even more blatant.

Is this a bug in Sema::LookupOrdinaryName ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support ns::f3, it seems that we need to use LookupNestedNameSpecifierName first to find ns, then find f3. The process is quite complex and may not fit into the specific mark* function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely there's another case where we need to go from mangled string back to named decl. Perhaps there's somewhere else in clang that already does so and can be reused here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may not fit

?

Copy link
Member Author

@MaskRay MaskRay Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fully support qualified names in alias/ifunc attributes for the -Wunused-function diagnostic, it would take a lot of code due to complexity of C++ name lookup when namespaces are involved.

This PR intends to address the ifunc -Wunused-function issue. The C++ support is a side product. And it would seem odd to add lots of code to fix a -Wunused-function false positive related to qualified names (which people likely don't use).

The test is added here to test the current behavior.

int g3() __attribute__((alias("_ZN2nsL2f3Ev")));
}

template <class T>
static void *f4(T) { return nullptr; }
static void *f4() { return nullptr; } // cxx-warning{{unused function 'f4'}}
extern void g4() __attribute__((ifunc("_ZL2f4IiEPvT_")));
void *use3 = f4(0);
#endif
1 change: 1 addition & 0 deletions utils/bazel/llvm-project-overlay/clang/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,7 @@ cc_library(
"//llvm:AllTargetsAsmParsers",
"//llvm:AllTargetsCodeGens",
"//llvm:Core",
"//llvm:Demangle",
"//llvm:FrontendHLSL",
"//llvm:FrontendOpenMP",
"//llvm:MC",
Expand Down