Remove function bitcast constants that are not linked in the IR

[mikerice]

Summary

There are some cases (in C involving unprototyped functions) where bitcasts
are created and not actually used in the IR. This leaves a use for the
function that isn't in the IR and breaks Function::hasAddressTaken().

The added tests show the cases broken by this.

A simple case like:

int foo() { return 1; }
int main() { return foo(); }

The bitcast is created but not used when simplifyVariadicCallee succeeds.

Similarly when an unprototyped declaration occurs followed by a definition:

void b();
void b() { }

The bitcast constant is added to the GlobalValReplacement list, but may
never actually be needed to replace anything.

For lack of a better idea, this change just keeps track of the possibly unused constants and removes them if not
used at the end of the module.

[rjmccall]

LLVM clients that want to do use-analyses of globals are supposed to call removeDeadConstantUsers() first. We could do that for every global coming out of IRGen, but I'm not sure that's a good use of compile time. Not creating unnecessary casts seems like at most a best-effort thing.

[mikerice]

Thanks for looking at this. We've found another case that event this patch doesn't handle so we'll try to deal with this some other way.

clang/lib/CodeGen/CGCall.cpp

auto simplifyVariadicCallee = [](llvm::FunctionType *CalleeFT,
     for (unsigned i = 0, e = OrigFT->getNumParams(); i != e; ++i)
       if (OrigFT->getParamType(i) != CalleeFT->getParamType(i))
         return nullptr;
 
     return OrigFn;
   };
 
   if (llvm::Function *OrigFn = simplifyVariadicCallee(IRFuncTy, CalleePtr)) {
+    if (llvm::ConstantExpr *CE = dyn_cast<llvm::ConstantExpr>(CalleePtr))
+      CGM.addUnusedConstant(CE);
     CalleePtr = OrigFn;
     IRFuncTy = OrigFn->getFunctionType();
   }
 
   // 3. Perform the actual call.
 
   // Deactivate any cleanups that we're supposed to do immediately before
   // the call.

clang/lib/CodeGen/CodeGenModule.h

private:
   ReplacementsTy Replacements;
 
   /// List of global values to be replaced with something else. Used when we
   /// want to replace a GlobalValue but can't identify it by its mangled name
   /// anymore (because the name is already taken).
   llvm::SmallVector<std::pair<llvm::GlobalValue *, llvm::Constant *>, 8>
     GlobalValReplacements;
 
+  /// A set containing constants that may not have been linked into the IR.
+  /// These bitcasts due to unprototyped functions can be skipped over or
+  /// replaced leaving unwanted function references if not removed.
+  llvm::SmallPtrSet<llvm::Constant *, 16> UnusedConstants;
+
   /// Variables for which we've emitted globals containing their constant
   /// values along with the corresponding globals, for opportunistic reuse.
   llvm::DenseMap<const VarDecl*, llvm::GlobalVariable*> InitializerConstants;
 
   /// Set of global decls for which we already diagnosed mangled name conflict.
   /// Required to not issue a warning (on a mangling conflict) multiple times
   /// for the same decl.
   llvm::DenseSet<GlobalDecl> DiagnosedConflictingDefinitions;
public:
   ///
   /// NOTE: This should only be called for definitions.
   void SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV);
 
   void addReplacement(StringRef Name, llvm::Constant *C);
 
   void addGlobalValReplacement(llvm::GlobalValue *GV, llvm::Constant *C);
 
+  void addUnusedConstant(llvm::Constant *C);
+
   /// Emit a code for threadprivate directive.
   /// \param D Threadprivate declaration.
   void EmitOMPThreadPrivateDecl(const OMPThreadPrivateDecl *D);
 
   /// Emit a code for declare reduction construct.
   void EmitOMPDeclareReduction(const OMPDeclareReductionDecl *D,
                                CodeGenFunction *CGF = nullptr);
 
private:
   void EmitVTablesOpportunistically();
 
   /// Call replaceAllUsesWith on all pairs in Replacements.
   void applyReplacements();
 
   /// Call replaceAllUsesWith on all pairs in GlobalValReplacements.
   void applyGlobalValReplacements();
 
+  void removeUnusedConstants();
+
   void checkAliases();
 
   std::map<int, llvm::TinyPtrVector<llvm::Function *>> DtorsUsingAtExit;
 
   /// Register functions annotated with __attribute__((destructor)) using
   /// __cxa_atexit, if it is available, or atexit otherwise.
   void registerGlobalDtorsWithAtExit();
 

clang/lib/CodeGen/CodeGenModule.cpp

 
 void CodeGenModule::applyGlobalValReplacements() {
   for (auto &I : GlobalValReplacements) {
     llvm::GlobalValue *GV = I.first;
     llvm::Constant *C = I.second;
 
     GV->replaceAllUsesWith(C);
     GV->eraseFromParent();
+    addUnusedConstant(C);
+  }
+}
+
+void CodeGenModule::addUnusedConstant(llvm::Constant *C) {
+  UnusedConstants.insert(C);
+}
+
+void CodeGenModule::removeUnusedConstants() {
+  for (auto *C : UnusedConstants) {
+    if (C->use_empty())
+      C->destroyConstant();
   }
 }
 
 // This is only used in aliases that we created and we know they have a
 // linear structure.
 static const llvm::GlobalValue *getAliasedGlobal(const llvm::GlobalValue *GV) {
   const llvm::Constant *C;
   if (auto *GA = dyn_cast<llvm::GlobalAlias>(GV))
static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
   }
 }
 
 void CodeGenModule::Release() {
   EmitDeferred();
   EmitVTablesOpportunistically();
   applyGlobalValReplacements();
   applyReplacements();
+  removeUnusedConstants();
   checkAliases();
   emitMultiVersionFunctions();
   EmitCXXGlobalInitFunc();
   EmitCXXGlobalCleanUpFunc();
   registerGlobalDtorsWithAtExit();
   EmitCXXThreadLocalInitFunc();
   if (ObjCRuntime)
     if (llvm::Function *ObjCInitFunction = ObjCRuntime->ModuleInitFunction())

clang/unittests/CodeGen/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Core
   Support
   )
 
 add_clang_unittest(ClangCodeGenTests
   BufferSourceTest.cpp
   CodeGenExternalTest.cpp
   TBAAMetadataTest.cpp
   CheckTargetFeaturesTest.cpp
+  FuncAddrTakenTest.cpp
   )
 
 clang_target_link_libraries(ClangCodeGenTests
   PRIVATE
   clangAST
   clangBasic
   clangCodeGen
   clangFrontend
   clangInterpreter
   clangLex
   clangParse
   clangSerialization
   )

clang/unittests/CodeGen/FuncAddrTakenTest.cpp

+//===- unittests/CodeGen/FuncAddrTakenTest.cpp - Function addr taken test -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestCompiler.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/CodeGen/ModuleBuilder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Parse/ParseAST.h"
+#include "clang/Sema/Sema.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/Support/Host.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+TEST(FuncAddrTakenTest, FuncAddrTakenTest1) {
+  const char TestProgram[] =
+    "int foo() { return 1; }\n"
+    "int func(void) { return 2; }\n"
+    "int bar() { return foo() + func(); }\n";
+
+  clang::LangOptions LO;
+  LO.C11 = 1;
+  TestCompiler Compiler(LO);
+  Compiler.init(TestProgram);
+
+  clang::ParseAST(Compiler.compiler.getSema(), false, false);
+  llvm::Module *M =
+      static_cast<clang::CodeGenerator &>(Compiler.compiler.getASTConsumer())
+          .GetModule();
+
+  llvm::Function *F = M->getFunction("foo");
+  ASSERT_TRUE(F != nullptr);
+
+  bool AddrTaken = F->hasAddressTaken();
+  ASSERT_FALSE(AddrTaken);
+
+  F = M->getFunction("func");
+  ASSERT_TRUE(F != nullptr);
+
+  AddrTaken = F->hasAddressTaken();
+  ASSERT_FALSE(AddrTaken);
+}
+
+TEST(FuncAddrTakenTest, FuncAddrTakenTest2) {
+  const char TestProgram[] =
+  "static char a[] = {};\n"
+  "void b();\n"
+  "static void c(char *d) { b(); }\n"
+  "int e() {\n"
+  "  c(a);\n"
+  "  return 1;\n"
+  "}\n"
+  "void f() { b(); }\n"
+  "void b() {}\n";
+
+  clang::LangOptions LO;
+  LO.C11 = 1;
+  TestCompiler Compiler(LO);
+  Compiler.init(TestProgram);
+
+  clang::ParseAST(Compiler.compiler.getSema(), false, false);
+  llvm::Module *M =
+      static_cast<clang::CodeGenerator &>(Compiler.compiler.getASTConsumer())
+          .GetModule();
+
+  llvm::Function *F = M->getFunction("b");
+  ASSERT_TRUE(F != nullptr);
+
+  bool AddrTaken = F->hasAddressTaken();
+  ASSERT_FALSE(AddrTaken);
+}
+} // end anonymous namespace