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

Revert "[Sema] Mark alias/ifunc targets used and consider mangled names" #88919

Conversation

joker-eph
Copy link
Collaborator

@joker-eph joker-eph added the skip-precommit-approval PR for CI feedback, not intended for review label Apr 16, 2024
@joker-eph joker-eph requested review from MaskRay and rupprecht April 16, 2024 15:44
@joker-eph joker-eph merged commit 51b42b7 into main Apr 16, 2024
5 of 6 checks passed
@joker-eph joker-eph deleted the revert-87130-users/MaskRay/spr/sema-mark-aliasifunc-targets-used-and-consider-mangled-names branch April 16, 2024 15:45
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" bazel "Peripheral" support tier build system: utils/bazel labels Apr 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 16, 2024

@llvm/pr-subscribers-clang

Author: Mehdi Amini (joker-eph)

Changes

Reverts llvm/llvm-project#87130

Bot is broken with clang crash: https://lab.llvm.org/buildbot/#/builders/272/builds/14063/steps/6/logs/stdio


Full diff: https://github.com/llvm/llvm-project/pull/88919.diff

6 Files Affected:

  • (modified) clang/lib/Sema/CMakeLists.txt (-1)
  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+11-33)
  • (modified) clang/test/AST/ast-dump-attr-json.cpp (-1)
  • (modified) clang/test/Sema/alias-unused-win.cpp (+1-1)
  • (modified) clang/test/Sema/alias-unused.cpp (+7-9)
  • (modified) utils/bazel/llvm-project-overlay/clang/BUILD.bazel (-1)
diff --git a/clang/lib/Sema/CMakeLists.txt b/clang/lib/Sema/CMakeLists.txt
index a96439df664228..ab3b813a9ccd97 100644
--- a/clang/lib/Sema/CMakeLists.txt
+++ b/clang/lib/Sema/CMakeLists.txt
@@ -1,6 +1,5 @@
 set(LLVM_LINK_COMPONENTS
   Core
-  Demangle
   FrontendHLSL
   FrontendOpenMP
   MC
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index d26f130b5774ce..b7b1fbc625a150 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -45,7 +45,6 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/STLForwardCompat.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"
@@ -1984,36 +1983,6 @@ 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 (not supported by microsoftDemangle
-// https://github.com/llvm/llvm-project/issues/88825). 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) {
-  std::unique_ptr<char, llvm::FreeDeleter> Demangled;
-  if (S.getASTContext().getCXXABIKind() != TargetCXXABI::Microsoft)
-    Demangled.reset(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.get() : 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);
-    }
-  }
-}
-
 static void handleIFuncAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   StringRef Str;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
@@ -2026,7 +1995,6 @@ 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));
 }
 
@@ -2061,7 +2029,17 @@ static void handleAliasAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
     }
   }
 
-  markUsedForAliasOrIfunc(S, D, AL, Str);
+  // 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);
+  }
+
   D->addAttr(::new (S.Context) AliasAttr(S.Context, AL, Str));
 }
 
diff --git a/clang/test/AST/ast-dump-attr-json.cpp b/clang/test/AST/ast-dump-attr-json.cpp
index 883e584bfedf07..051c2956abfdf7 100644
--- a/clang/test/AST/ast-dump-attr-json.cpp
+++ b/clang/test/AST/ast-dump-attr-json.cpp
@@ -46,7 +46,6 @@ __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": {
diff --git a/clang/test/Sema/alias-unused-win.cpp b/clang/test/Sema/alias-unused-win.cpp
index 97d57a3bbd1e31..47c96d41175179 100644
--- a/clang/test/Sema/alias-unused-win.cpp
+++ b/clang/test/Sema/alias-unused-win.cpp
@@ -7,7 +7,7 @@ extern "C" {
 static int f(void) { return 42; } // cxx-warning{{unused function 'f'}}
 int g(void) __attribute__((alias("f")));
 
-static int foo [] = { 42, 0xDEAD };
+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 __attribute__((overloadable)) f0(int x) { return x; } // expected-warning{{unused function 'f0'}}
diff --git a/clang/test/Sema/alias-unused.cpp b/clang/test/Sema/alias-unused.cpp
index c0b541c880e525..dc8e46f072d74d 100644
--- a/clang/test/Sema/alias-unused.cpp
+++ b/clang/test/Sema/alias-unused.cpp
@@ -14,26 +14,24 @@ extern typeof(foo) bar __attribute__((unused, alias("foo")));
 /// We report a warning in C++ mode because the internal linkage `resolver` gets
 /// mangled as it does not have a language linkage. GCC does not mangle
 /// `resolver` or report a warning.
-static int (*resolver(void))(void) { return f; } // cxx-warning{{unused function 'resolver'}}
+static int (*resolver(void))(void) { return f; } // expected-warning{{unused function 'resolver'}}
 int ifunc(void) __attribute__((ifunc("resolver")));
 
-static int __attribute__((overloadable)) f0(int x) { return x; }
+static int __attribute__((overloadable)) f0(int x) { return x; } // expected-warning{{unused function 'f0'}}
 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; }
+static int f1() { return 42; } // expected-warning{{unused function 'f1'}}
 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; }
+static int f2(int) { return 42; } // expected-warning{{unused function 'f2'}}
+static int f2() { return 42; } // expected-warning{{unused function 'f2'}}
 int g2() __attribute__((alias("_ZL2f2v")));
 
-static int (*resolver1())() { return f; } // cxx-warning{{unused function 'resolver1'}}
-static int (*resolver1(int))() { return f; }
+static int (*resolver1())() { return f; } // expected-warning{{unused function 'resolver1'}}
+static int (*resolver1(int))() { return f; } // expected-warning{{unused function 'resolver1'}}
 int ifunc1() __attribute__((ifunc("_ZL9resolver1i")));
 
 /// TODO: We should report "unused function" for f3(int).
diff --git a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel b/utils/bazel/llvm-project-overlay/clang/BUILD.bazel
index 725ac6bb38120b..c2f77e3abca0e6 100644
--- a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1136,7 +1136,6 @@ cc_library(
         "//llvm:AllTargetsAsmParsers",
         "//llvm:AllTargetsCodeGens",
         "//llvm:Core",
-        "//llvm:Demangle",
         "//llvm:FrontendHLSL",
         "//llvm:FrontendOpenMP",
         "//llvm:MC",

@MaskRay
Copy link
Member

MaskRay commented Apr 16, 2024

Thanks. 1c2afba is a codegen test of this involved llvm-libc pattern which would also catch the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category skip-precommit-approval PR for CI feedback, not intended for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants