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.

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

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

Merged
merged 9 commits into from
Apr 16, 2024
+46 −19
Merged

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

Show file tree
Hide file tree
Changes from 2 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
31 changes: 20 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,23 @@ 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. For C++, we look up the demangled name
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also happen in C because of __attribute__((overloadable)) (so the code is correct, but we should fix up the comment and add a test case in C for this situation)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for mentioning overloadable. Added a test and fixed the comment.

// ignoring parameters. This should handle the majority of use cases while
// leaveing false positives for namespace scope names and false negatives in
// the presence of overloads.
static void markUsedForAliasOrIfunc(Sema &S, Decl *D, const ParsedAttr &AL,
StringRef Str) {
char *Demangled = llvm::itaniumDemangle(Str, /*ParseParams=*/false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't really assume 'itanium demangle' here, this could be a non-itanium build.

Copy link
Member

Choose a reason for hiding this comment

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

How does one specify a "non-itanium build?"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Triple is the only way I am aware of IIRC. So we should be checking that triple. I think ifunc is ELF/Itanium only, and I'm unsure about alias. That said, I'm aware of at least 1 downstream (IIRC) that used an ELF system with an MSVC mangling (one of the offload compilers).

Copy link
Member

@nickdesaulniers nickdesaulniers Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like there's getLangOpts().CXXABI. TargetCXXABI::Microsoft is the one that doesn't use itanium FWICT (looking at ASTContext::createCXXABI) And TargetInfo::getCXXABI (looking at ASTContext::createMangleContext). isItaniumFamily comes up, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there's getLangOpts().CXXABI. TargetCXXABI::Microsoft is the one that doesn't use itanium FWICT (looking at ASTContext::createCXXABI) And TargetInfo::getCXXABI (looking at ASTContext::createMangleContext). isItaniumFamily comes up, too.

Yep, Microsoft is the exception. We DO typically use https://clang.llvm.org/doxygen/classclang_1_1TargetCXXABI.html to determine which.

Copy link
Member Author

@MaskRay MaskRay Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

alias(mangled) referring to non-existent definitions are rejected by another diagnostic.

It's an error

% fclang -c b.cc
b.cc:3:28: error: ifunc must point to a defined function
    3 | int i1(int) __attribute__((ifunc("f")));
      |                            ^
b.cc:3:28: note: the function specified in an ifunc must refer to its mangled name
b.cc:3:28: note: function by that name is mangled as "_Z1fv"
    3 | int i1(int) __attribute__((ifunc("f")));
      |                            ^~~~~~~~~~
      |                            ifunc("_Z1fv")
1 error generated.

The reported issue has nothing to do with security as all. It's about a -Wunused-function false positive. (https://git.tukaani.org/?p=xz.git;a=commitdiff;h=a37a2763383e6c204fe878e1416dd35e7711d3a9)

The warning (under -Wall) becomes an error if users provide -Werror, not what Linux distributions do.
In addition, the backdoor is skipped unless GCC is used.

Copy link
Member

Choose a reason for hiding this comment

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

While we can add S.getASTContext().getCXXABIKind() != TargetCXXABI::Microsoft check for downstream users, it doesn't appear to add any value?

If we know the target ABI uses Microsoft mangling, then we should be using microsoftDemangle rather than itaniumDemangle. There's also llvm::demangle which can try to figure this out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, I agree that we should not be assuming itanium demangling here; we definitely should care about Microsoft mangling as well because alias is supported on all targets, including Windows.

The reported issue has nothing to do with security as all. It's about a -Wunused-function false positive.

Eh, I don't agree. Replacing false positives with false negatives can be a security concern because the false negatives could be hiding an issue. @nickdesaulniers pointed out a potential in https://github.com/llvm/llvm-project/pull/87130/files#r1546691566. (The reverse can also be true -- replacing false negatives with false positives can hide security issues because low-quality output may either cause the user to disable the diagnostic entirely or may drown out the valid issues.)

Regardless, I think the direction of this patch is heading where we want it to go. But urging caution in this area is very reasonable IMO.

Copy link
Member Author

@MaskRay MaskRay Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added a S.getASTContext().getCXXABIKind() != TargetCXXABI::Microsoft check.

Note: while alias works for Windows, it is only checked in tests CodeGenCXX/visibility-dllstorageclass.cpp/CodeGen/debug-info-alias-pointer.c . I don't consider an internal linkage target used or cared by users.

I've also updated the description to state

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.

extern "C" {
static void f0() {}
// GCC: void g0() __attribute__((alias("_ZL2f0v")));
// Clang: void g0() __attribute__((alias("f0")));
}

This makes internal linkage target difficult to use in C++ even on Linux.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a S.getASTContext().getCXXABIKind() != TargetCXXABI::Microsoft check.

Note: while alias works for Windows, it is only checked in tests CodeGenCXX/visibility-dllstorageclass.cpp/CodeGen/debug-info-alias-pointer.c . I don't consider an internal linkage target used or cared by users.

I'm not certain why there's so much resistance to doing this when three people have pointed out the concern. It still works for Windows. e.g.,

F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
namespace NS {
  extern "C" int puts(const char *);
  void f() { puts("I did the thing!"); }
}

void func() __attribute__((alias("?f@NS@@YAXXZ")));

int main() {
  func();
}

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"

F:\source\llvm-project>a.exe
I did the thing!

We should support it properly rather than only halfway, especially considering the context of the bug report.

nickdesaulniers marked this conversation as resolved.
Show resolved Hide resolved
if (Demangled)
Str = Demangled;
erichkeane marked this conversation as resolved.
Show resolved Hide resolved
const DeclarationNameInfo target(&S.Context.Idents.get(Str), AL.getLoc());
LookupResult LR(S, target, Sema::LookupOrdinaryName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const DeclarationNameInfo target(&S.Context.Idents.get(Str), AL.getLoc());
LookupResult LR(S, target, Sema::LookupOrdinaryName);
DeclarationNameInfo Target(&S.Context.Idents.get(Str), AL.getLoc());
LookupResult LR(S, Target, Sema::LookupOrdinaryName);

Coding style nits

if (S.LookupQualifiedName(LR, S.getCurLexicalContext()))
for (NamedDecl *ND : LR)
ND->markUsed(S.Context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This loop doesn't seem quite right either. First, we need to ensure we're only marking 'functions' as used, not all possible named decls I think.

Second, we need to ensure we're doing a better job with testing, this doesn't seem to be adding enough test infrastructure to make sure we're picking up the right function.

Finally, what does this mean in the case of template declarations/instantiations? We need to make sure we're marking those correctly (or not picking them up at all perhaps?).

Copy link
Member

Choose a reason for hiding this comment

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

I believe you can put alias attributes on non-functions: https://godbolt.org/z/s1PbojYr4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you can put alias attributes on non-functions: https://godbolt.org/z/s1PbojYr4.

ifuncs cannot, so we perhaps need to be more careful there.

Copy link
Member Author

@MaskRay MaskRay Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

"alias" can target variables. "ifunc" has an existing diagnostic to reject non-function targets.

% cat a.c
static int x;
extern void y() __attribute__((ifunc("x")));
% clang -c a.c
a.c:2:32: error: ifunc must point to a defined function
    2 | extern void y() __attribute__((ifunc("x")));
      |                                ^
1 error generated.

I am adding a template test:

template <class T>
static void *f3(T) { return nullptr; }
static void *f3() { return nullptr; } // expected-warning{{unused function 'f3'}}
extern void g3() __attribute__((ifunc("_ZL2f3IiEPvT_")));
void *use3 = f3(0);

I think the current code does the right thing for templates because mangled names for templates contain <>.

% c++filt -p <<< _ZL2f3IiEPvT_
f3<int>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of the nature of ifunc and alias, would it make sense to have an assertion here that what we found was either a variable or a function, just to be defensive?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added code to check the mangled name to avoid -Wunused-function false negatives.

free(Demangled);
}

static void handleIFuncAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
StringRef Str;
if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
Expand All @@ -1992,6 +2010,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 +2045,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
38 changes: 37 additions & 1 deletion clang/test/Sema/alias-unused.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,43 @@
// RUN: %clang_cc1 -triple x86_64-linux-gnu -Wunneeded-internal-declaration -x c -verify %s
// RUN: %clang_cc1 -triple x86_64-linux-gnu -Wunused -x c -verify %s
// RUN: %clang_cc1 -triple x86_64-linux-gnu -Wunused -x c++ -verify %s

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see a non-Itanium RUN line as well. Perhaps we should be using -triple %itanium_abi_triple and -triple %ms_abi_triple rather than a specific target?

#ifdef __cplusplus
extern "C" {
#else
// expected-no-diagnostics
#endif
static int f(void) { return 42; }
int g(void) __attribute__((alias("f")));

static int foo [] = { 42, 0xDEAD };
extern typeof(foo) bar __attribute__((unused, alias("foo")));

static int (*resolver(void))(void) { return f; }
int ifunc(void) __attribute__((ifunc("resolver")));

#ifdef __cplusplus
}

/// We demangle alias/ifunc target and mark all found functions as used.
/// We should report "unused function" for f1(int).
static int f1(int) { return 42; }
static int f1(void) { return 42; }
int g1() __attribute__((alias("_ZL2f1v")));

/// We should report "unused function" for resolver1(int).
static int (*resolver1(void))(void) { return f; }
static int (*resolver1(int))(void) { return f; }
int ifunc1() __attribute__((ifunc("_ZL9resolver1v")));

namespace ns {
static int f2(int) { return 42; } // expected-warning{{unused function 'f2'}}
static int f2(void) { return 42; } // expected-warning{{unused function 'f2'}}
int g2() __attribute__((alias("_ZN2nsL2f2Ev")));
Copy link
Member

Choose a reason for hiding this comment

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

Why warn for f2(void)? Would _ZNL2nsL2f2Ev be the mangling? (an additional L between N and 2)?

Copy link
Member Author

Choose a reason for hiding this comment

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

c++filt -p <<< _ZNL2nsL2f2Ev => ns::f2. We attempt to find ns::f2 in the scope and will find nothing (we should use f2).

This weakness is mentioned in the description as "... does not handle namespace scope names"

Copy link
Member

Choose a reason for hiding this comment

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

LookupParsedName smells like it might handle namespaced lookups?

}

template <class T>
static void *f3(T) { return nullptr; }
static void *f3() { return nullptr; } // expected-warning{{unused function 'f3'}}
extern void g3() __attribute__((ifunc("_ZL2f3IiEPvT_")));
void *use3 = f3(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
Loading