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

[Sean Silva via llvm-commits]

silvas added a comment.

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)


================
Comment at: lib/profile/InstrProfilingBuffer.c:51
@@ +50,3 @@
+    memcpy(Buffer, IOVecs[I].Data, Length);
+    *(char **)&Buffer += Length;
+  }
----------------
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);
```

================
Comment at: lib/profile/InstrProfilingFile.c:20
@@ -19,4 +19,3 @@
 
-static size_t fileWriter(const void *Data, size_t ElmSize, size_t NumElm,
-                         void **File) {
-  return fwrite(Data, ElmSize, NumElm, (FILE *)*File);
+/* Return if there is an error, otherwise return  0.  */
+static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,
----------------
I think this is missing "1" in "Return 1 if there is an error". The comment in bufferWriter is similarly affected.

================
Comment at: lib/profile/InstrProfilingFile.c:22
@@ +21,3 @@
+static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,
+                           void *File) {
+  uint32_t I;
----------------
I would rename the argument `void *WriterCtx` and then have a declaration `FILE *File = (FILE *)WriterCtx;` as the first thing in the function.



http://reviews.llvm.org/D14859