[InstrProfiling] Don't attempt to create duplicate data variables. #71998
Conversation
Fixes a bug introduced by commit f95b2f1 ("Reland [InstrProf][compiler-rt] Enable MC/DC Support in LLVM Source-based Code Coverage (1/3)")
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-pgo Author: Alan Phipps (evodius96) ChangesFixes a bug introduced by createDataVariable() needs to check that a data variable wasn't already created before creating it. Previously, this was done inadvertantly in getOrCreateRegionCounters(), which checked that the RegionCounters was not created multiple times before creating the counter section and the data variable. When the creation of the data variable was abstracted into its own function (createDataVariable()), there was no corresponding check. This was failing on a case in which an instrumented function was being inlined into multiple functions and a duplicate data variable was created, which led to a segfault in emitNameData(). Test case added based on the repro that also ensures a single data variable was created in this case. Full diff: https://github.com/llvm/llvm-project/pull/71998.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index eadf1cd9baccc60..c426a15eeb0bafb 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -1251,6 +1251,10 @@ void InstrProfiling::createDataVariable(InstrProfCntrInstBase *Inc,
GlobalVariable *NamePtr = Inc->getName();
auto &PD = ProfileDataMap[NamePtr];
+ // Return if data variable was already created.
+ if (PD.DataVar)
+ return;
+
LLVMContext &Ctx = M->getContext();
Function *Fn = Inc->getParent()->getParent();
diff --git a/llvm/test/Instrumentation/InstrProfiling/inline-data-var.ll b/llvm/test/Instrumentation/InstrProfiling/inline-data-var.ll
new file mode 100644
index 000000000000000..dcb97d0a9b9761e
--- /dev/null
+++ b/llvm/test/Instrumentation/InstrProfiling/inline-data-var.ll
@@ -0,0 +1,26 @@
+;; Check that only one data variable is created when an instrprof.increment is
+;; inlined into more than one function.
+; RUN: opt %s -passes='cgscc(inline),instrprof' -S | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: @__profd_foobar = private global
+; CHECK-NOT @__profd_foobar
+
+declare void @llvm.instrprof.increment(ptr %0, i64 %1, i32 %2, i32 %3)
+@__profn_foobar = private constant [6 x i8] c"foobar"
+
+define internal void @foobar() {
+ call void @llvm.instrprof.increment(ptr @__profn_foobar, i64 123456, i32 32, i32 0)
+ ret void
+}
+
+define void @foo() {
+ call void @foobar()
+ ret void
+}
+
+define void @bar() {
+ call void @foobar()
+ ret void
+}
|
|
Using the repro I observed another bug where the data variable may not always be created for some inlined functions under certain conditions. I'm testing a fix that I'll put in a separate PR soon. |
Can you quickly comment on the impact of that bug? Does it mean the compiler crashes, miscompiles, or loses coverage information? |
The missing data variable may result in lost coverage information for functions fully inlined into other instrumented functions. If inlining happens after the profiling pass, there’s no problem, but that’s obviously a faulty assumption. The solution is to move where the data variable is created and also ensure that MC/DC parameter information is processed first. I think the fix is also straightforward, and I should have the PR tomorrow. |
|
Created #72069 for the other bug I observed. Thank you for your assistance with this. |
…lvm#71998) Fixes a bug introduced by commit f95b2f1 ("Reland [InstrProf][compiler-rt] Enable MC/DC Support in LLVM Source-based Code Coverage (1/3)") createDataVariable() needs to check that a data variable wasn't already created before creating it. Previously, this was done inadvertantly in getOrCreateRegionCounters(), which checked that the RegionCounters was not created multiple times before creating the counter section and the data variable. When the creation of the data variable was abstracted into its own function (createDataVariable()), there was no corresponding check. This was failing on a case in which an instrumented function was being inlined into multiple functions and a duplicate data variable was created, which led to a segfault in emitNameData(). Test case added based on the repro that also ensures a single data variable was created in this case.
Fixes a bug introduced by
commit f95b2f1 ("Reland [InstrProf][compiler-rt] Enable MC/DC Support in LLVM Source-based Code Coverage (1/3)")
createDataVariable() needs to check that a data variable wasn't already created before creating it. Previously, this was done inadvertantly in getOrCreateRegionCounters(), which checked that the RegionCounters was not created multiple times before creating the counter section and the data variable. When the creation of the data variable was abstracted into its own function (createDataVariable()), there was no corresponding check. This was failing on a case in which an instrumented function was being inlined into multiple functions and a duplicate data variable was created, which led to a segfault in emitNameData(). Test case added based on the repro that also ensures a single data variable was created in this case.