[InstrProf] Don't take the address of alwaysinline available_externally functions

[vsk]

Summary

Doing so breaks compilation of the following C program
(under -fprofile-instr-generate):

__attribute__((always_inline)) inline int foo() { return 0; }

int main() { return foo(); }

At link time, we fail because taking the address of an
available_externally function creates an undefined external reference,
which the TU cannot provide.

Emitting the function definition into the object file at all appears to
be a violation of the langref: "Globals with 'available_externally'
linkage are never emitted into the object file corresponding to the LLVM
module."

Testing: check-llvm, check-profile

[davidxl]

lib/Transforms/Instrumentation/InstrProfiling.cpp

346

Should you check always inline attribute here too to avoid missing indirect call profiling? AvailableExternally is very common in C++ (e.g, for extern template). Blindly dropping the address can lead to lots of missing value profiling.

475

This can not be readonly -- there is a pointer to the value profiling counter array from here.

[vsk]

• Only change behavior when handling alwaysinline + available_externally functions.

[davidxl]

lgtm ( The area of linkage handling has many subtle problems in the past with large C++ apps, so be prepared for any breakage introduced).

lib/Transforms/Instrumentation/InstrProfiling.cpp

static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   SmallVector<char, 24> HashPostfix;
   if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
     return (Prefix + Name).str();
   return (Prefix + Name + "." + Twine(FuncHash)).str();
 }
 
 static inline bool shouldRecordFunctionAddr(Function *F) {
   // Check the linkage
-  if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage() &&
-      !F->hasAvailableExternallyLinkage())
+  if (F->hasAvailableExternallyLinkage())
+    return false;
+  if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage())
     return true;
   // Prohibit function address recording if the function is both internal and
   // COMDAT. This avoids the profile data variable referencing internal symbols
   // in COMDAT.
   if (F->hasLocalLinkage() && F->hasComdat())
     return false;
   // Check uses of this function for other than direct calls or invokes to it.
   // Inline virtual functions have linkeOnceODR linkage. When a key method
#include "llvm/ProfileData/InstrProfData.inc"
   Constant *Int16ArrayVals[IPVK_Last + 1];
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
 
   Constant *DataVals[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
-  auto *Data = new GlobalVariable(*M, DataTy, false, NamePtr->getLinkage(),
-                                  ConstantStruct::get(DataTy, DataVals),
-                                  getVarName(Inc, getInstrProfDataVarPrefix()));
+  auto *Data = new GlobalVariable(
+      *M, DataTy, /*isConstant=*/isa<ConstantPointerNull>(FunctionAddr),
+      NamePtr->getLinkage(), ConstantStruct::get(DataTy, DataVals),
   Data->setVisibility(NamePtr->getVisibility());
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));
   Data->setAlignment(INSTR_PROF_DATA_ALIGNMENT);
   Data->setComdat(ProfileVarsComdat);
 
   PD.RegionCounters = CounterPtr;
   PD.DataVar = Data;
   ProfileDataMap[NamePtr] = PD;

test/Instrumentation/InstrProfiling/linkage.ll

 ; COMMON: @__profc_foo_weak = weak hidden global
 ; COMMON: @__profd_foo_weak = weak hidden global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; COMMON: @"__profc_linkage.ll:foo_internal" = internal global
-; COMMON: @"__profd_linkage.ll:foo_internal" = internal global
+; COMMON: @"__profd_linkage.ll:foo_internal" = internal constant
 define internal void @foo_internal() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([23 x i8], [23 x i8]* @"__profn_linkage.ll:foo_internal", i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; COMMON: @__profc_foo_inline = linkonce_odr hidden global
 ; COMMON: @__profd_foo_inline = linkonce_odr hidden global
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; LINUX: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat($__profv_foo_extern), align 8
-; LINUX: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8
+; LINUX: @__profd_foo_extern = linkonce_odr hidden constant {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8
 ; OTHER: @__profc_foo_extern = linkonce_odr hidden global
-; OTHER: @__profd_foo_extern = linkonce_odr hidden global
+; OTHER: @__profd_foo_extern = linkonce_odr hidden constant
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 ; OTHER: @__llvm_profile_runtime = external global i32

test/Transforms/PGOProfile/comdat_internal.ll

 ; RUN: opt < %s -pgo-instr-gen -instrprof -S | FileCheck %s
 ; RUN: opt < %s -passes=pgo-instr-gen,instrprof -S | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $foo = comdat any
 ; CHECK: $foo = comdat any
 
 ; CHECK: $__llvm_profile_raw_version = comdat any
 ; CHECK: $__profv__stdin__foo.[[FOO_HASH:[0-9]+]] = comdat any
 
 @bar = global i32 ()* @foo, align 8
 
 ; CHECK: @__llvm_profile_raw_version = constant i64 {{[0-9]+}}, comdat
 ; CHECK-NOT: __profn__stdin__foo
 ; CHECK: @__profc__stdin__foo.[[FOO_HASH]] = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
-; CHECK: @__profd__stdin__foo.[[FOO_HASH]] = private global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 -5640069336071256030, i64 [[FOO_HASH]], i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo.[[FOO_HASH]], i32 0, i32 0), i8* null
+; CHECK: @__profd__stdin__foo.[[FOO_HASH]] = private constant { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 -5640069336071256030, i64 [[FOO_HASH]], i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo.[[FOO_HASH]], i32 0, i32 0), i8* null
 ; CHECK-NOT: bitcast (i32 ()* @foo to i8*)
 ; CHECK-SAME: , i8* null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
 ; CHECK: @__llvm_prf_nm
 ; CHECK: @llvm.used
 
 define internal i32 @foo() comdat {
 entry:
   ret i32 1
 }