Fixed __llvm_profile_write_buffer in presence of ValueProfileData. #97350
Conversation
__llvm_profile_write_buffer generated invalid profile files in presence of non empty ValueProfileData. Fixed that by precomputing the size of ValueProfileData, adding it to the total size, and then filling that.
|
@llvm/pr-subscribers-pgo Author: Sunil Srivastava (sks75220) Changes__llvm_profile_write_buffer generated invalid profile files in presence of non empty ValueProfileData. Fixed that by precomputing the size of ValueProfileData, adding it to the total size, and then filling that. The new test, instrprof-write-buffer.c is an example of the failure. The changes to instrprof-without-libc.c are a bit questionable. I don't understand the purpose of this test, but the new code pulls in three function that this test does not like. I don't see an easy way to avoid this change (instrprof-without-libc.c), though it may be possible to achieve that by splitting some files to ensure that the new changes are not pulled in this test. Full diff: https://github.com/llvm/llvm-project/pull/97350.diff 5 Files Affected:
diff --git a/compiler-rt/lib/profile/InstrProfilingBuffer.c b/compiler-rt/lib/profile/InstrProfilingBuffer.c
index 1c451d7ec7563..b4c3399477068 100644
--- a/compiler-rt/lib/profile/InstrProfilingBuffer.c
+++ b/compiler-rt/lib/profile/InstrProfilingBuffer.c
@@ -220,12 +220,19 @@ uint64_t __llvm_profile_get_size_for_buffer_internal(
&PaddingBytesAfterNames, &PaddingBytesAfterVTable,
&PaddingBytesAfterVNames);
+ /* Compute size of ValueProfData. */
+ int64_t VPDSize = __llvm_profile_getSizeOfValueProfData(
+ lprofGetVPDataReader(), DataBegin, DataEnd);
+ if (VPDSize < 0) {
+ /* Got error marker. We will not increase the size below. */
+ VPDSize = 0;
+ }
return sizeof(__llvm_profile_header) + __llvm_write_binary_ids(NULL) +
DataSize + PaddingBytesBeforeCounters + CountersSize +
PaddingBytesAfterCounters + NumBitmapBytes +
PaddingBytesAfterBitmapBytes + NamesSize + PaddingBytesAfterNames +
VTableSize + PaddingBytesAfterVTable + VNameSize +
- PaddingBytesAfterVNames;
+ PaddingBytesAfterVNames + VPDSize;
}
COMPILER_RT_VISIBILITY
@@ -237,7 +244,18 @@ void initBufferWriter(ProfDataWriter *BufferWriter, char *Buffer) {
COMPILER_RT_VISIBILITY int __llvm_profile_write_buffer(char *Buffer) {
ProfDataWriter BufferWriter;
initBufferWriter(&BufferWriter, Buffer);
- return lprofWriteData(&BufferWriter, 0, 0);
+
+ /* Pass VPDataReader hook, if it will not fail. */
+ const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
+ const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
+ VPDataReaderType *Reader = lprofGetVPDataReader();
+ if (__llvm_profile_getSizeOfValueProfData(Reader,DataBegin, DataEnd) < 0) {
+ /* Attempt to use lprofGetVPDataReader will result in an error, so
+ * do not use it. Generate profile data without VPDataReader.
+ */
+ Reader = 0;
+ }
+ return lprofWriteData(&BufferWriter, Reader, 0);
}
COMPILER_RT_VISIBILITY int __llvm_profile_write_buffer_internal(
diff --git a/compiler-rt/lib/profile/InstrProfilingInternal.h b/compiler-rt/lib/profile/InstrProfilingInternal.h
index d5bd0e41fb129..0bf642b54f89f 100644
--- a/compiler-rt/lib/profile/InstrProfilingInternal.h
+++ b/compiler-rt/lib/profile/InstrProfilingInternal.h
@@ -79,6 +79,8 @@ typedef struct ProfBufferIO {
uint32_t BufferSz;
/* Current byte offset from the start of the buffer. */
uint32_t CurOffset;
+ /* The number of bytes already flushed. */
+ uint64_t FlushedSize;
} ProfBufferIO;
/* The creator interface used by testing. */
@@ -169,6 +171,11 @@ void lprofMergeValueProfData(struct ValueProfData *SrcValueProfData,
VPDataReaderType *lprofGetVPDataReader();
+/* Compute size of VPData. Return -1 on error (0 is a valid return value). */
+int64_t __llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader,
+ const __llvm_profile_data *DataBegin,
+ const __llvm_profile_data *DataEnd);
+
/* Internal interface used by test to reset the max number of
* tracked values per value site to be \p MaxVals.
*/
diff --git a/compiler-rt/lib/profile/InstrProfilingWriter.c b/compiler-rt/lib/profile/InstrProfilingWriter.c
index 8816a71155511..4cfa2d1e96fbe 100644
--- a/compiler-rt/lib/profile/InstrProfilingWriter.c
+++ b/compiler-rt/lib/profile/InstrProfilingWriter.c
@@ -59,6 +59,7 @@ static void llvmInitBufferIO(ProfBufferIO *BufferIO, ProfDataWriter *FileWriter,
BufferIO->BufferStart = Buffer;
BufferIO->BufferSz = BufferSz;
BufferIO->CurOffset = 0;
+ BufferIO->FlushedSize = 0;
}
COMPILER_RT_VISIBILITY ProfBufferIO *
@@ -108,6 +109,12 @@ lprofBufferIOWrite(ProfBufferIO *BufferIO, const uint8_t *Data, uint32_t Size) {
}
COMPILER_RT_VISIBILITY int lprofBufferIOFlush(ProfBufferIO *BufferIO) {
+ /* If this is a 'counter' buffer, just keep track of how much got flushed. */
+ if (BufferIO->FileWriter == NULL) {
+ BufferIO->FlushedSize += BufferIO->CurOffset;
+ BufferIO->CurOffset = 0;
+ return 0;
+ }
if (BufferIO->CurOffset) {
ProfDataIOVec IO[] = {
{BufferIO->BufferStart, sizeof(uint8_t), BufferIO->CurOffset, 0}};
@@ -214,6 +221,37 @@ static int writeOneValueProfData(ProfBufferIO *BufferIO,
return 0;
}
+/* Compute size of VPData, without actually writing it. This function follows
+ * the logic of writeValueProfData below it. It is used for predicting how big
+ * a buffer is needed to store the ValueProfileData.
+ * Note that this function returns -1 as an error indicator and 0 is
+ * a potentially valid value.
+ */
+int64_t
+__llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader,
+ const __llvm_profile_data *DataBegin,
+ const __llvm_profile_data *DataEnd) {
+ const __llvm_profile_data *DI = 0;
+ ProfBufferIO *BufferIO = lprofCreateBufferIO(NULL);
+ // This BufferIO is a special struct just for counting how much is being
+ // 'written'. The backing Writer is NULL. It collects number of bytes
+ // in CurOffset, and finally in FlushedSize.
+
+ for (DI = DataBegin; DI < DataEnd; DI++) {
+ if (writeOneValueProfData(BufferIO, VPDataReader, DI)) {
+ return -1;
+ }
+ }
+ if (lprofBufferIOFlush(BufferIO) != 0)
+ return -1;
+
+ int64_t rv = BufferIO->FlushedSize;
+ lprofDeleteBufferIO(BufferIO);
+ return rv;
+}
+
+
+
static int writeValueProfData(ProfDataWriter *Writer,
VPDataReaderType *VPDataReader,
const __llvm_profile_data *DataBegin,
diff --git a/compiler-rt/test/profile/instrprof-without-libc.c b/compiler-rt/test/profile/instrprof-without-libc.c
index 3142138cdffc0..dcec4cd756f77 100644
--- a/compiler-rt/test/profile/instrprof-without-libc.c
+++ b/compiler-rt/test/profile/instrprof-without-libc.c
@@ -66,13 +66,7 @@ int main(int argc, const char *argv[]) {
// CHECK-SYMBOLS-NOT: {{ }}_fdopen
// CHECK-SYMBOLS-NOT: {{ }}_fopen
// CHECK-SYMBOLS-NOT: {{ }}_fwrite
-// CHECK-SYMBOLS-NOT: {{ }}_getenv
-// CHECK-SYMBOLS-NOT: {{ }}getenv
// CHECK-SYMBOLS-NOT: {{ }}_malloc
// CHECK-SYMBOLS-NOT: {{ }}malloc
-// CHECK-SYMBOLS-NOT: {{ }}_calloc
-// CHECK-SYMBOLS-NOT: {{ }}calloc
-// CHECK-SYMBOLS-NOT: {{ }}_free
-// CHECK-SYMBOLS-NOT: {{ }}free
// CHECK-SYMBOLS-NOT: {{ }}_open
// CHECK-SYMBOLS-NOT: {{ }}_getpagesize
diff --git a/compiler-rt/test/profile/instrprof-write-buffer.c b/compiler-rt/test/profile/instrprof-write-buffer.c
new file mode 100755
index 0000000000000..79fba78394ccd
--- /dev/null
+++ b/compiler-rt/test/profile/instrprof-write-buffer.c
@@ -0,0 +1,42 @@
+// UNSUPPORTED: target={{.*windows.*}}
+// This test is derived from "compiler-rt/test/profile/instrprof-write-buffer-internal.c",
+// and that test was disabled on Windows due to sanitizer-windows bot failures.
+// Doing the same here. See 2fcc3f4b18.
+//
+// RUN: rm -f %t.buf.profraw
+// RUN: %clang_pgogen -o %t %s
+// RUN: %run %t %t.buf.profraw
+// RUN: llvm-profdata show %t.buf.profraw
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+void threadFunc(void* callback) // Needed for failure.
+{
+ typedef void (FuncPtr)();
+ (*(FuncPtr*)callback)();
+}
+
+uint64_t __llvm_profile_get_size_for_buffer();
+int __llvm_profile_write_buffer(char*);
+
+int main(int argc, const char *argv[])
+{
+ // Write to a buffer, and write that to a file.
+ uint64_t bufsize = __llvm_profile_get_size_for_buffer();
+ char *buf = malloc(bufsize);
+ int ret = __llvm_profile_write_buffer(buf);
+
+ if (ret != 0) {
+ fprintf(stderr, "failed to write buffer");
+ return ret;
+ }
+
+ FILE *f = fopen(argv[1], "w");
+ fwrite(buf, bufsize, 1, f);
+ fclose(f);
+ free(buf);
+
+ return 0;
+}
|
You can test this locally with the following command:git-clang-format --diff 938cbdb4cf428bf08558c24d845aeac9174c7022 522509ec372fc3442fb4055e915d9d6256e46d69 -- compiler-rt/test/profile/instrprof-write-buffer.c compiler-rt/lib/profile/InstrProfilingBuffer.c compiler-rt/lib/profile/InstrProfilingInternal.h compiler-rt/lib/profile/InstrProfilingWriter.c compiler-rt/test/profile/instrprof-without-libc.cView the diff from clang-format here.diff --git a/compiler-rt/lib/profile/InstrProfilingInternal.h b/compiler-rt/lib/profile/InstrProfilingInternal.h
index 0bf642b54f..0bb7b86c64 100644
--- a/compiler-rt/lib/profile/InstrProfilingInternal.h
+++ b/compiler-rt/lib/profile/InstrProfilingInternal.h
@@ -172,7 +172,8 @@ void lprofMergeValueProfData(struct ValueProfData *SrcValueProfData,
VPDataReaderType *lprofGetVPDataReader();
/* Compute size of VPData. Return -1 on error (0 is a valid return value). */
-int64_t __llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader,
+int64_t
+__llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader,
const __llvm_profile_data *DataBegin,
const __llvm_profile_data *DataEnd);
diff --git a/compiler-rt/lib/profile/InstrProfilingWriter.c b/compiler-rt/lib/profile/InstrProfilingWriter.c
index 4cfa2d1e96..bb7a911aec 100644
--- a/compiler-rt/lib/profile/InstrProfilingWriter.c
+++ b/compiler-rt/lib/profile/InstrProfilingWriter.c
@@ -250,8 +250,6 @@ __llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader,
return rv;
}
-
-
static int writeValueProfData(ProfDataWriter *Writer,
VPDataReaderType *VPDataReader,
const __llvm_profile_data *DataBegin,
diff --git a/compiler-rt/test/profile/instrprof-write-buffer.c b/compiler-rt/test/profile/instrprof-write-buffer.c
index 9e80575ab8..5417771df4 100755
--- a/compiler-rt/test/profile/instrprof-write-buffer.c
+++ b/compiler-rt/test/profile/instrprof-write-buffer.c
@@ -17,17 +17,16 @@
#include <stdio.h>
#include <stdlib.h>
-void threadFunc(void* callback) // Needed for failure.
+void threadFunc(void *callback) // Needed for failure.
{
- typedef void (FuncPtr)();
- (*(FuncPtr*)callback)();
+ typedef void(FuncPtr)();
+ (*(FuncPtr *)callback)();
}
uint64_t __llvm_profile_get_size_for_buffer();
-int __llvm_profile_write_buffer(char*);
+int __llvm_profile_write_buffer(char *);
-int main(int argc, const char *argv[])
-{
+int main(int argc, const char *argv[]) {
// Write to a buffer, and write that to a file.
uint64_t bufsize = __llvm_profile_get_size_for_buffer();
char *buf = malloc(bufsize);
|
There was a problem hiding this comment.
I'm still reading the implementation. To clarify about the high-level goal, does this patch intend to add value profiling support for __llvm_profile_write_buffer (which is absent before)? And would you mind elaborating on how is __llvm_profile_write_buffer is invoked in your use case?
| // and that test was disabled on Windows due to sanitizer-windows bot failures. | ||
| // Doing the same here. See 2fcc3f4b18. | ||
| // | ||
| // RUN: rm -f %t.buf.profraw |
There was a problem hiding this comment.
Add CHECK lines to show value profiles are generated.
There was a problem hiding this comment.
Added CHECK lines for llvm-profdata output, but is there any other way to show value profiles are generated ? llvm-profdata does fail without code changes.
There was a problem hiding this comment.
I patched this PR and I can see llvm-profdata show returns truncated profile data without the code change.
Suggest to show some function counters. For instance, --all-functions -counts -ic-targets which gives the output below. --function=main should limit function counters to main.
./bin/llvm-profdata show --all-functions --counts -ic-targets /path/to/instrprof-write-buffer.c.tmp.buf.profraw
Counters:
threadFunc:
Hash: 0x025f5c817fffffff
Counters: 1
Indirect Call Site Count: 1
Block counts: [0]
Indirect Target Results:
main:
Hash: 0x0209aa3e3852da94
Counters: 2
Indirect Call Site Count: 0
Block counts: [0, 0]
Indirect Target Results:
Instrumentation level: IR entry_first = 0
Functions shown: 2
Total functions: 2
Maximum function count: 0
Maximum internal block count: 0
Statistics for indirect call sites profile:
Total number of sites: 1
Total number of sites with values: 0
Total number of profiled values: 0
Value sites histogram:
NumTargets, SiteCount
Basically, the test program calls __llvm_profile_write_buffer(buf) to generate profdata in a buffer, and then writes that buffer to the file via plain fwrite. That should generate a valid profraw file, but it does not in this case. That is the bug being fixed here. buf = malloc(bufsize = __llvm_profile_get_size_for_buffer()); The generated file is not accepted by 'llvm-profdata show' or merge. They fail with : |
| // and that test was disabled on Windows due to sanitizer-windows bot failures. | ||
| // Doing the same here. See 2fcc3f4b18. | ||
| // | ||
| // RUN: rm -f %t.buf.profraw |
There was a problem hiding this comment.
I patched this PR and I can see llvm-profdata show returns truncated profile data without the code change.
Suggest to show some function counters. For instance, --all-functions -counts -ic-targets which gives the output below. --function=main should limit function counters to main.
./bin/llvm-profdata show --all-functions --counts -ic-targets /path/to/instrprof-write-buffer.c.tmp.buf.profraw
Counters:
threadFunc:
Hash: 0x025f5c817fffffff
Counters: 1
Indirect Call Site Count: 1
Block counts: [0]
Indirect Target Results:
main:
Hash: 0x0209aa3e3852da94
Counters: 2
Indirect Call Site Count: 0
Block counts: [0, 0]
Indirect Target Results:
Instrumentation level: IR entry_first = 0
Functions shown: 2
Total functions: 2
Maximum function count: 0
Maximum internal block count: 0
Statistics for indirect call sites profile:
Total number of sites: 1
Total number of sites with values: 0
Total number of profiled values: 0
Value sites histogram:
NumTargets, SiteCount
| @@ -66,13 +66,7 @@ int main(int argc, const char *argv[]) { | |||
| // CHECK-SYMBOLS-NOT: {{ }}_fdopen | |||
| // CHECK-SYMBOLS-NOT: {{ }}_fopen | |||
| // CHECK-SYMBOLS-NOT: {{ }}_fwrite | |||
| // CHECK-SYMBOLS-NOT: {{ }}_getenv | |||
There was a problem hiding this comment.
My understanding of @david-xl 's comment is that these lines check the absence of a certain set of functions to make sure __llvm_profile_write_buffer doesn't introduce dependencies to libc by default.
Put it another way, the fact we need to remove these lines means the current patch would introduce dependency to libc (getenv is from libc according to https://man7.org/linux/man-pages/man3/getenv.3.html). I wonder if gating code paths with C macros and keeping them not defined by default is the way to go.
There was a problem hiding this comment.
the current patch would introduce dependency to libc
Yes, I see the point. I am trying to see if I can avoid that. It should be possible, but may require some more work.
There was a problem hiding this comment.
Yes, please definitely avoid any dependency on libc.
| * a potentially valid value. | ||
| */ | ||
| int64_t | ||
| __llvm_profile_getSizeOfValueProfData(VPDataReaderType *VPDataReader, |
There was a problem hiding this comment.
nit: use camelCase for consistency with the rest of the function in InstrProfilingWriter.c
getValueProfSize sounds good enough to me.
| @@ -214,6 +221,37 @@ static int writeOneValueProfData(ProfBufferIO *BufferIO, | |||
| return 0; | |||
| } | |||
|
|
|||
| /* Compute size of VPData, without actually writing it. This function follows | |||
| * the logic of writeValueProfData below it. It is used for predicting how big | |||
| * a buffer is needed to store the ValueProfileData. | |||
There was a problem hiding this comment.
nit: If possible, introduce a common helper function and call it from both __llvm_profile_getSizeOfValueProfData and writeValueProfData to de-duplicate logic.
| @@ -0,0 +1,47 @@ | |||
| // UNSUPPORTED: target={{.*windows.*}} | |||
| // This test is derived from "compiler-rt/test/profile/instrprof-write-buffer-internal.c", | |||
There was a problem hiding this comment.
Hmm, I added threadFunc [1] to compiler-rt/test/profile/instrprof-write-buffer-internal.c without the non-test code changes in this patch, and llvm-profdata show can parse the profiles [2].
On the other hand, without the non-test code changes I can see profile truncate error for this test case. I need to dig and find an explanation for that. Meanwhile let me know if you happen to have a theory already.
[1]
diff --git a/compiler-rt/test/profile/instrprof-write-buffer-internal.c b/compiler-rt/test/profile/instrprof-write-buffer-internal.c
index 2c1c29ac0c58..a610abca2495 100644
--- a/compiler-rt/test/profile/instrprof-write-buffer-internal.c
+++ b/compiler-rt/test/profile/instrprof-write-buffer-internal.c
@@ -41,6 +41,12 @@ int __llvm_profile_write_buffer_internal(
void __llvm_profile_set_dumped(void);
+void threadFunc(void *callback) // Needed for failure.
+{
+ typedef void(FuncPtr)();
+ (*(FuncPtr *)callback)();
+}
+
int main(int argc, const char *argv[]) {
uint64_t bufsize = __llvm_profile_get_size_for_buffer_internal(
__llvm_profile_begin_data(), __llvm_profile_end_data(),
[2]
bin/llvm-profdata show /path/to/instrprof-write-buffer-internal.c.tmp.buf.profraw
There was a problem hiding this comment.
On the other hand, without the non-test code changes I can see profile truncate error for this test case.
Meanwhile let me know if you happen to have a theory already.
I don't have a theory. I admit that I don't fully understand the profraw format, or the concept of value-profile-data.
This bug started from a customer report who came across an invalid profraw generated by __llvm_profile_write_buffer() (rejected by llvm-profdata), whereas a similar use of __llvm_profile_write_file() worked fine.
So I have tried to mimic the functionality of __llvm_profile_write_file into __llvm_profile_write_buffer, but I have ended up adding dependencies that are best avoided.
__llvm_profile_write_buffer generated invalid profile files in presence of non empty ValueProfileData. Fixed that by precomputing the size of ValueProfileData, adding it to the total size, and then filling that.
The new test, instrprof-write-buffer.c is an example of the failure.
The changes to instrprof-without-libc.c are a bit questionable. I don't understand the purpose of this test, but the new code pulls in three function that this test does not like.
I don't see an easy way to avoid this change (instrprof-without-libc.c), though it may be possible to achieve that by splitting some files to ensure that the new changes are not pulled in this test.
I am open to suggestions.