[PGO] Prohibit the recording the function address if it's internal and COMDAT.

[xur]

Summary

For COMDAT functions, to remove the duplicate counters, we create a "__profv_*" COMDAT group for the profile data objects, so that only one set of profile data objects will be emitted in the final object.

This works fine until the we have the value profiling. In indirect-call value profile we record the function address to be used in the runtime. This is problematic for COMDAT functions with internal linkage (we do create such function, like global initializer). If we record the address of such function, We will create a reference to internal symbols in COMDAT. This violates the standard.

One can use the following two .ll files to reproduce the issue:

• a.ll ----

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

$_Z3foov = comdat any

@fp1 = global i32 ()* @_Z3foov, align 8

define internal i32 @_Z3foov() comdat {
entry:

ret i32 1

}

• end of b.ll ----

• b.ll ----

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

$_Z3foov = comdat any

@fp2 = global i32 ()* @_Z3foov, align 8

define internal i32 @_Z3foov() comdat {
entry:

ret i32 1

}

• end of b.ll ----

clang a.ll -c && clang b.ll -c && clang a.o b.o

We have the following warning:
"warning: relocation refers to discarded section"

This patch prohibits the recording the function address if it's internal and COMDAT.

[xur]

repost the .ll file:

a.ll:

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

$_Z3foov = comdat any

@fp1 = global i32 ()* @_Z3foov, align 8

define internal i32 @_Z3foov() comdat {
entry:
  ret i32 1
}`

b.ll:

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

$_Z3foov = comdat any

@fp2 = global i32 ()* @_Z3foov, align 8

define internal i32 @_Z3foov() comdat {
entry:
  ret i32 1
}

[davidxl]

can you make a test out of a.ll -- by checking the per function data not referencing foo after instrumentation?

[xur]

Add a test case.

[xur]

fix comments

[betulb]

Thanks for the test.

lib/Transforms/Instrumentation/InstrProfiling.cpp

241 ↗

(On Diff #55033)

I do not know yet much about COMDAT's. I need to read about them. Otherwise, the check is fine. But, I'd prefer it to be placed after line 244. There we reduce the linkages to check for to one of linkonce, local or available externally. I'd also like to check if the same warning shows up for either of linkonce or available externally linkages.

[xur]

lib/Transforms/Instrumentation/InstrProfiling.cpp

241 ↗

(On Diff #55033)

I have no problem moving it down since these two checks are exclusive for now.
But I'm wondering why your prefer the different order.
To me, "return false" check should ahead "return true" as this is a correctness issue.

other linkages do not seem to matter here.

[betulb]

lib/Transforms/Instrumentation/InstrProfiling.cpp

241 ↗

(On Diff #55033)

Thanks. My thought was had the other linkages mattered, the COMDAT check could have been merged into the final return statement as:

return !F->hasComdat() && F->hasAddressTaken();

[xur]

This is different from my patch, right? For linkoncelinage and comdat and
address taken, we do want to record the address.

This version will not, however.

Rong

[davidxl]

Move the new check after the existing return true check. Can be revisited later if it is a problem.

lgtm

test/Transforms/PGOProfile/comdat_internal.ll

15 ↗

(On Diff #55033)

Do you want to explicitly have a CHECK-NOT here? That way the test is more robust.

[xur]

will move the check down in the commit.

test/Transforms/PGOProfile/comdat_internal.ll

15 ↗

(On Diff #55033)

we check if the field set to 0 (ie. i8* null), rather a bitcast of foo's address.
I think this is enough.

[davidxl]

test/Transforms/PGOProfile/comdat_internal.ll

15 ↗

(On Diff #55033)

That is not good enough in a sense that other people may not know the intention of this 0 value and update the case with non zero value when this is broken.

[xur]

This is the updated patch.
(1) moving the check down.
(2) add a check-not to the test case.

If no objection, I'll commit the change this afternoon.

Thank you all.

-Rong

[davidxl]

test/Transforms/PGOProfile/comdat_internal.ll

16 ↗

(On Diff #55273)

I think you should relax the all the irrelevant substrings inside, and just keep @__profd__stdin__foo and @foo in the pattern.

[xur]

I did not find an easy way to do the check-not match with the sub string.
; CHECK-NOT: @__profd__stdin__foo foo
obviously not working.

the best approximation I can think of is to use CHECK-NOT with CHECK-SAME.

[davidxl]

Ok.

David

[krasin]

FYI: this revision has likely broken bots:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/12318

FAIL: LLVM :: Transforms/PGOProfile/comdat_internal.ll (15333 of 16582)
******************** TEST 'LLVM :: Transforms/PGOProfile/comdat_internal.ll' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/./bin/opt < /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/test/Transforms/PGOProfile/comdat_internal.ll -pgo-instr-gen -instrprof -S | /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/./bin/FileCheck /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/test/Transforms/PGOProfile/comdat_internal.ll
--
Exit Code: 1

Command Output (stderr):
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/test/Transforms/PGOProfile/comdat_internal.ll:19:10: error: expected string not found in input
; CHECK: @__llvm_prf_nm = private constant [21 x i8] c"\0B\13x\DA\B3).I\C9\CC\B3\B3J\CB\CF\07\00\18a\04\1B", section "__llvm_prf_names"
         ^
<stdin>:17:1: note: scanning from here
@__llvm_prf_nm = private constant [13 x i8] c"\0B\00<stdin>:foo", section "__llvm_prf_names"
^

[xur]

fixed after r267816.

Thanks,

-Rong

[krasin]

Thank you. And sorry for the late alarm. It seems that my message was posted after the fix was submitted.

llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp

static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   return (Prefix + Name).str();
 }
 
 static inline bool shouldRecordFunctionAddr(Function *F) {
   // Check the linkage
   if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage() &&
       !F->hasAvailableExternallyLinkage())
     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.
   return F->hasAddressTaken();
 }
 
 static inline bool needsComdatForCounter(Function &F, Module &M) {
 
   if (F.hasComdat())
     return true;

llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll

+; RUN: opt < %s -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: $__llvm_profile_raw_version = comdat any
+; CHECK: $__profv__stdin__foo = comdat any
+
+@bar = global i32 ()* @foo, align 8
+
+; CHECK: @__llvm_profile_raw_version = constant i64 {{[0-9]+}}, comdat
+; CHECK: @__profn__stdin__foo = private constant [11 x i8] c"<stdin>:foo"
+; CHECK: @__profc__stdin__foo = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profv__stdin__foo), align 8
+; CHECK: @__profd__stdin__foo = private global { i64, i64, i64*, i8*, i8*, i32, [1 x i16] } { i64 -5640069336071256030, i64 12884901887, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo, i32 0, i32 0), i8*
+; CHECK-NOT: bitcast (i32 ()* @foo to i8*)
+; CHECK-SAME: null
+; CHECK-SAME: , i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo), align 8
+; CHECK: @__llvm_prf_nm = private constant [21 x i8] c"\0B\13x\DA\B3).I\C9\CC\B3\B3J\CB\CF\07\00\18a\04\1B", section "__llvm_prf_names"
+; CHECK: @llvm.used = appending global [2 x i8*] [i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [1 x i16] }* @__profd__stdin__foo to i8*), i8* getelementptr inbounds ([21 x i8], [21 x i8]* @__llvm_prf_nm, i32 0, i32 0)], section "llvm.metadata"
+
+define internal i32 @foo() comdat {
+entry:
+  ret i32 1
+}