This is an archive of the discontinued LLVM Phabricator instance.

Differential D19515

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

Authored by xur on Apr 25 2016, 5:20 PM.

Details

Reviewers
davidxl
betulb
Commits
rGaf5aebaa3246: [PGO] Prohibit address recording if the function is both internal and COMDAT
rL267792: [PGO] Prohibit address recording if the function is both internal and COMDAT
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.

Diff Detail

Event Timeline

xur updated this revision to Diff 54950.Apr 25 2016, 5:20 PM
xur retitled this revision from to [PGO] Prohibit the recording the function address if it's internal and COMDAT..
xur updated this object.
xur added reviewers: davidxl, betulb.
xur added subscribers: llvm-commits, xur.
xur added a comment.Apr 25 2016, 5:22 PM

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 edited edge metadata.Apr 25 2016, 10:47 PM

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

xur updated this revision to Diff 55027.Apr 26 2016, 10:07 AM
xur edited edge metadata.

Add a test case.

xur updated this revision to Diff 55033.Apr 26 2016, 10:24 AM

fix comments

betulb edited edge metadata.Apr 26 2016, 11:11 AM

Thanks for the test.

lib/Transforms/Instrumentation/InstrProfiling.cpp
236

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 added inline comments.Apr 26 2016, 11:39 AM
lib/Transforms/Instrumentation/InstrProfiling.cpp
236

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 added inline comments.Apr 26 2016, 12:19 PM
lib/Transforms/Instrumentation/InstrProfiling.cpp
236

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 added a comment.Apr 26 2016, 12:53 PM

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 accepted this revision.Apr 27 2016, 10:23 AM
davidxl edited edge metadata.

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
16

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

This revision is now accepted and ready to land.Apr 27 2016, 10:23 AM
xur added a comment.Apr 27 2016, 11:31 AM

will move the check down in the commit.

test/Transforms/PGOProfile/comdat_internal.ll
16

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

davidxl added inline comments.Apr 27 2016, 11:38 AM
test/Transforms/PGOProfile/comdat_internal.ll
16

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 updated this revision to Diff 55273.Apr 27 2016, 11:47 AM
xur edited edge metadata.

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 added inline comments.Apr 27 2016, 11:50 AM
test/Transforms/PGOProfile/comdat_internal.ll
16

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

xur updated this revision to Diff 55302.Apr 27 2016, 1:18 PM

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 added a subscriber: davidxl.Apr 27 2016, 1:41 PM

Ok.

David

Closed by commit rL267792: [PGO] Prohibit address recording if the function is both internal and COMDAT (authored by xur). · Explain WhyApr 27 2016, 2:23 PM
This revision was automatically updated to reflect the committed changes.
krasin added a subscriber: krasin.Apr 27 2016, 5:33 PM

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 added a comment.Apr 27 2016, 6:11 PM

fixed after r267816.

Thanks,

-Rong

krasin added a comment.Apr 27 2016, 6:22 PM

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

Revision Contents

PathSize
lib/
Transforms/
Instrumentation/
5 lines
test/
Transforms/
PGOProfile/
23 lines

Diff 55273

lib/Transforms/Instrumentation/InstrProfiling.cpp

Show First 20 Lines • Show All 227 Lines • ▼ Show 20 Lines
/// Get the name of a profiling variable for a particular function. /// Get the name of a profiling variable for a particular function.
static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) { static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
StringRef NamePrefix = getInstrProfNameVarPrefix(); StringRef NamePrefix = getInstrProfNameVarPrefix();
StringRef Name = Inc->getName()->getName().substr(NamePrefix.size()); StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
return (Prefix + Name).str(); return (Prefix + Name).str();
} }
static inline bool shouldRecordFunctionAddr(Function *F) { static inline bool shouldRecordFunctionAddr(Function *F) {
// Check the linkage // Check the linkage
betulbUnsubmitted
Not Done
ReplyInline Actions

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.

betulb: I do not know yet much about COMDAT's. I need to read about them. Otherwise, the check is fine.
xurAuthorUnsubmitted
Not Done
ReplyInline Actions

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.

xur: I have no problem moving it down since these two checks are exclusive for now. But I'm…
betulbUnsubmitted
Not Done
ReplyInline Actions

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();

betulb: Thanks. My thought was had the other linkages mattered, the COMDAT check could have been merged…
if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage() && if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage() &&
!F->hasAvailableExternallyLinkage()) !F->hasAvailableExternallyLinkage())
return true; 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. // Check uses of this function for other than direct calls or invokes to it.
return F->hasAddressTaken(); return F->hasAddressTaken();
} }
static inline bool needsComdatForCounter(Function &F, Module &M) { static inline bool needsComdatForCounter(Function &F, Module &M) {
if (F.hasComdat()) if (F.hasComdat())
return true; return true;
▲ Show 20 Lines • Show All 281 Lines • Show Last 20 Lines

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* null, i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo), align 8
; CHECK-NOT: @__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* bitcast (i32 ()* @foo to i8*), i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo), align 8
davidxlUnsubmitted
Not Done
ReplyInline Actions

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

davidxl: Do you want to explicitly have a CHECK-NOT here? That way the test is more robust.
xurAuthorUnsubmitted
Not Done
ReplyInline Actions

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

xur: we check if the field set to 0 (ie. i8* null), rather a bitcast of foo's address. I think this…
davidxlUnsubmitted
Not Done
ReplyInline Actions

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.

davidxl: That is not good enough in a sense that other people may not know the intention of this 0 value…
davidxlUnsubmitted
Not Done
ReplyInline Actions

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

davidxl: I think you should relax the all the irrelevant substrings inside, and just keep…
; 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
}