[PGO] Implement a more robust/readable Writer callback interface

[davidxl]

Summary

Patch suggested by Sean.

[silvas]

Thanks, this is a lot cleaner!
I've made some stylistic suggestion to help move the unsafe casting to highly-visible locations, which I think reveals a logic bug (2 potential fixes suggested), but with that fixed this LGTM.

(also a tiny comment typo fix)

lib/profile/InstrProfilingBuffer.c

51 ↗

(On Diff #40740)

I would rename the argument void *WriterCtx and then have a declaration char *Buffer = (char *)WriterCtx; as the first thing in the function.

But with that change, it becomes clearer that bufferWriter does not maintain the correct state across calls (do we have a test that fails? if not it would be nice to add one). I think as far as making this code clear and boring, I would suggest:

struct BufferWriterCtx {
  char *BufPtr;
};

... inside bufferWriter ...
  BufferWriterCtx *Ctx = (BufferWriterCtx *)WriterCtx;
... inside the loop
    Ctx->BufPtr += Length;

You could probably get by using just char ** instead of a BufferWriterCtx struct, but then you would end up with an expression like char **PtrToBufPtr = (char **)WriterCtx; and (*PtrToBufPtr) += Length which is harder to follow.

Another possibility is for the writer to be called exactly once in all cases by changing llvmWriteProfDataImpl to be something like this:

ProfDataIOVec IOVecs[MAX_IOVECS] = {0};
ProfDataIOVec *NextIOVec = &IOVec[0];
appendIOVec(&NextIOVec, (const char *)&Header, sizeof(__llvm_profile_header), 1);
...
if (ValueDataBegin)
  appendIOVec(&NextIOVec, ValueDataBegin, sizeof(char), ValueDataSize);

Writer(IOVec, NextIOVec - &IOVec[0], WriterCtx);

lib/profile/InstrProfilingFile.c

20 ↗

(On Diff #40740)

I think this is missing "1" in "Return 1 if there is an error". The comment in bufferWriter is similarly affected.

22 ↗

(On Diff #40740)

I would rename the argument void *WriterCtx and then have a declaration FILE *File = (FILE *)WriterCtx; as the first thing in the function.

[davidxl]

I addressed the comments and committed the patch. Good observation about
maintaining writer state across calls to the callback -- I ended passing
pointer to writerCtx pointer to the callback so that it can be updated if
needed. It is not ideal, but also not reduce readability of the code.

thanks,

David

compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c

uint64_t __llvm_profile_get_size_for_buffer_internal(
   /* Match logic in __llvm_profile_write_buffer(). */
   const uint64_t NamesSize = PROFILE_RANGE_SIZE(Names) * sizeof(char);
   const uint8_t Padding = __llvm_profile_get_num_padding_bytes(NamesSize);
   return sizeof(__llvm_profile_header) +
          PROFILE_RANGE_SIZE(Data) * sizeof(__llvm_profile_data) +
          PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) + NamesSize + Padding;
 }
 
-static size_t bufferWriter(const void *Data, size_t ElmSize, size_t NumElm,
-                           void **Buffer) {
-  size_t Length = ElmSize * NumElm;
-  memcpy(*Buffer, Data, Length);
-  *(char **)Buffer += Length;
-  return NumElm;
+/* The buffer writer is reponsponsible in keeping writer state
+ * across the call.
+ */
+static uint32_t bufferWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,
+                             void **WriterCtx) {
+  uint32_t I;
+  char **Buffer = (char **)WriterCtx;
+  for (I = 0; I < NumIOVecs; I++) {
+    size_t Length = IOVecs[I].ElmSize * IOVecs[I].NumElm;
+    memcpy(*Buffer, IOVecs[I].Data, Length);
+    *Buffer += Length;
+  }
+  return 0;
 }
 
 __attribute__((visibility("hidden"))) int
 __llvm_profile_write_buffer(char *Buffer) {
-  return llvmWriteProfData(Buffer, bufferWriter, 0, 0);
+  return llvmWriteProfData(bufferWriter, Buffer, 0, 0);
 }
 
 __attribute__((visibility("hidden"))) int __llvm_profile_write_buffer_internal(
     char *Buffer, const __llvm_profile_data *DataBegin,
     const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,
     const uint64_t *CountersEnd, const char *NamesBegin, const char *NamesEnd) {
-  return llvmWriteProfDataImpl(Buffer, bufferWriter, DataBegin, DataEnd,
+  return llvmWriteProfDataImpl(bufferWriter, Buffer, DataBegin, DataEnd,
                                CountersBegin, CountersEnd, 0, 0, NamesBegin,
                                NamesEnd);
 }

compiler-rt/trunk/lib/profile/InstrProfilingFile.c

 #include "InstrProfilingUtil.h"
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
 #define UNCONST(ptr) ((void *)(uintptr_t)(ptr))
 
-static size_t fileWriter(const void *Data, size_t ElmSize, size_t NumElm,
-                         void **File) {
-  return fwrite(Data, ElmSize, NumElm, (FILE *)*File);
+/* Return 1 if there is an error, otherwise return  0.  */
+static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,
+                           void **WriterCtx) {
+  uint32_t I;
+  FILE *File = (FILE *)*WriterCtx;
+  for (I = 0; I < NumIOVecs; I++) {
+    if (fwrite(IOVecs[I].Data, IOVecs[I].ElmSize, IOVecs[I].NumElm, File) !=
+        IOVecs[I].NumElm)
+      return 1;
+  }
+  return 0;
 }
-uint8_t *ValueDataBegin = NULL;
 
 static int writeFile(FILE *File) {
   uint8_t *ValueDataBegin = NULL;
   const uint64_t ValueDataSize =
       __llvm_profile_gather_value_data(&ValueDataBegin);
-  int r = llvmWriteProfData(File, fileWriter, ValueDataBegin, ValueDataSize);
+  int r = llvmWriteProfData(fileWriter, File, ValueDataBegin, ValueDataSize);
   free(ValueDataBegin);
   return r;
 }
 
 static int writeFileWithName(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
   if (!OutputName || !OutputName[0])

compiler-rt/trunk/lib/profile/InstrProfilingInternal.h

 int __llvm_profile_write_buffer_internal(
     char *Buffer, const __llvm_profile_data *DataBegin,
     const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,
     const uint64_t *CountersEnd, const char *NamesBegin, const char *NamesEnd);
 
 /*!
  * This is an internal function not intended to be used externally.
  */
-typedef size_t (*WriterCallback)(const void *Data, size_t ElmS, size_t NumElm,
-                                 void **BufferOrFile);
-int llvmWriteProfData(void *WriterCtx, WriterCallback Writer,
+typedef struct ProfDataIOVec {
+  const char *Data;
+  size_t ElmSize;
+  size_t NumElm;
+} ProfDataIOVec;
+
+typedef uint32_t (*WriterCallback)(ProfDataIOVec *, uint32_t NumIOVecs,
+                                   void **WriterCtx);
+int llvmWriteProfData(WriterCallback Writer, void *WriterCtx,
                       const uint8_t *ValueDataBegin,
                       const uint64_t ValueDataSize);
-int llvmWriteProfDataImpl(void *WriterCtx, WriterCallback Writer,
+int llvmWriteProfDataImpl(WriterCallback Writer, void *WriterCtx,
                           const __llvm_profile_data *DataBegin,
                           const __llvm_profile_data *DataEnd,
                           const uint64_t *CountersBegin,
                           const uint64_t *CountersEnd,
                           const uint8_t *ValueDataBegin,
                           const uint64_t ValueDataSize, const char *NamesBegin,
                           const char *NamesEnd);
 
 #endif

compiler-rt/trunk/lib/profile/InstrProfilingWriter.c

 /*===- InstrProfilingWriter.c - Write instrumentation to a file or buffer -===*\
 |*
 |*                     The LLVM Compiler Infrastructure
 |*
 |* This file is distributed under the University of Illinois Open Source
 |* License. See LICENSE.TXT for details.
 |*
 \*===----------------------------------------------------------------------===*/
 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 
 __attribute__((visibility("hidden"))) int
-llvmWriteProfData(void *WriterCtx, WriterCallback Writer,
+llvmWriteProfData(WriterCallback Writer, void *WriterCtx,
                   const uint8_t *ValueDataBegin, const uint64_t ValueDataSize) {
   /* Match logic in __llvm_profile_write_buffer(). */
   const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
   const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
   const uint64_t *CountersBegin = __llvm_profile_begin_counters();
   const uint64_t *CountersEnd = __llvm_profile_end_counters();
   const char *NamesBegin = __llvm_profile_begin_names();
   const char *NamesEnd = __llvm_profile_end_names();
-  return llvmWriteProfDataImpl(WriterCtx, Writer, DataBegin, DataEnd,
+  return llvmWriteProfDataImpl(Writer, WriterCtx, DataBegin, DataEnd,
                                CountersBegin, CountersEnd, ValueDataBegin,
                                ValueDataSize, NamesBegin, NamesEnd);
 }
 
 __attribute__((visibility("hidden"))) int llvmWriteProfDataImpl(
-    void *WriterCtx, WriterCallback Writer,
+    WriterCallback Writer, void *WriterCtx,
     const __llvm_profile_data *DataBegin, const __llvm_profile_data *DataEnd,
     const uint64_t *CountersBegin, const uint64_t *CountersEnd,
     const uint8_t *ValueDataBegin, const uint64_t ValueDataSize,
     const char *NamesBegin, const char *NamesEnd) {
 
   /* Calculate size of sections. */
   const uint64_t DataSize = DataEnd - DataBegin;
   const uint64_t CountersSize = CountersEnd - CountersBegin;
__attribute__((visibility("hidden"))) int llvmWriteProfDataImpl(
   Header.CountersSize = CountersSize;
   Header.NamesSize = NamesSize;
   Header.CountersDelta = (uintptr_t)CountersBegin;
   Header.NamesDelta = (uintptr_t)NamesBegin;
   Header.ValueKindLast = IPVK_Last;
   Header.ValueDataSize = ValueDataSize;
   Header.ValueDataDelta = (uintptr_t)ValueDataBegin;
 
   /* Write the data. */
-#define CHECK_write(Data, Size, Length, BuffOrFile)                            \
-  do {                                                                         \
-    if (Writer(Data, Size, Length, &BuffOrFile) != Length)                     \
-      return -1;                                                               \
-  } while (0)
-  CHECK_write(&Header, sizeof(__llvm_profile_header), 1, WriterCtx);
-  CHECK_write(DataBegin, sizeof(__llvm_profile_data), DataSize, WriterCtx);
-  CHECK_write(CountersBegin, sizeof(uint64_t), CountersSize, WriterCtx);
-  CHECK_write(NamesBegin, sizeof(char), NamesSize, WriterCtx);
-  CHECK_write(Zeroes, sizeof(char), Padding, WriterCtx);
-  if (ValueDataBegin)
-    CHECK_write(ValueDataBegin, sizeof(char), ValueDataSize, WriterCtx);
-#undef CHECK_write
+  ProfDataIOVec IOVec[] = {
+      {(const char *)&Header, sizeof(__llvm_profile_header), 1},
+      {(const char *)DataBegin, sizeof(__llvm_profile_data), DataSize},
+      {(const char *)CountersBegin, sizeof(uint64_t), CountersSize},
+      {(const char *)NamesBegin, sizeof(char), NamesSize},
+      {(const char *)Zeroes, sizeof(char), Padding}};
+  if (Writer(IOVec, sizeof(IOVec) / sizeof(ProfDataIOVec), &WriterCtx))
+    return -1;
+  if (ValueDataBegin) {
+    ProfDataIOVec IOVec[1] = {{ValueDataBegin, sizeof(char), ValueDataSize}};
+    if (Writer(IOVec, 1, &WriterCtx))
+      return -1;
+  }
   return 0;
 }