<div dir="ltr">Thanks!<div><br></div><div>Do you have any ideas about tests we could add that would catch this issue? It seems like this would overwrite the header and would be rejected by llvm-profdata, so it should be easy to test.</div><div><br></div><div>-- Sean Silva</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 20, 2015 at 8:22 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>thanks,</div><div><br></div><div>David</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 20, 2015 at 7:36 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">silvas added a comment.<br>
<br>
Thanks, this is a lot cleaner!<br>
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.<br>
<br>
(also a tiny comment typo fix)<br>
<br>
<br>
================<br>
Comment at: lib/profile/InstrProfilingBuffer.c:51<br>
@@ +50,3 @@<br>
+    memcpy(Buffer, IOVecs[I].Data, Length);<br>
+    *(char **)&Buffer += Length;<br>
+  }<br>
----------------<br>
I would rename the argument `void *WriterCtx` and then have a declaration `char *Buffer = (char *)WriterCtx;` as the first thing in the function.<br>
<br>
<br>
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:<br>
<br>
```<br>
struct BufferWriterCtx {<br>
  char *BufPtr;<br>
};<br>
<br>
... inside bufferWriter ...<br>
  BufferWriterCtx *Ctx = (BufferWriterCtx *)WriterCtx;<br>
... inside the loop<br>
    Ctx->BufPtr += Length;<br>
```<br>
<br>
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.<br>
<br>
Another possibility is for the writer to be called exactly once in all cases by changing `llvmWriteProfDataImpl` to be something like this:<br>
<br>
```<br>
ProfDataIOVec IOVecs[MAX_IOVECS] = {0};<br>
ProfDataIOVec *NextIOVec = &IOVec[0];<br>
appendIOVec(&NextIOVec, (const char *)&Header, sizeof(__llvm_profile_header), 1);<br>
...<br>
if (ValueDataBegin)<br>
  appendIOVec(&NextIOVec, ValueDataBegin, sizeof(char), ValueDataSize);<br>
<br>
Writer(IOVec, NextIOVec - &IOVec[0], WriterCtx);<br>
```<br>
<br>
================<br>
Comment at: lib/profile/InstrProfilingFile.c:20<br>
@@ -19,4 +19,3 @@<br>
<br>
-static size_t fileWriter(const void *Data, size_t ElmSize, size_t NumElm,<br>
-                         void **File) {<br>
-  return fwrite(Data, ElmSize, NumElm, (FILE *)*File);<br>
+/* Return if there is an error, otherwise return  0.  */<br>
+static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,<br>
----------------<br>
I think this is missing "1" in "Return 1 if there is an error". The comment in bufferWriter is similarly affected.<br>
<br>
================<br>
Comment at: lib/profile/InstrProfilingFile.c:22<br>
@@ +21,3 @@<br>
+static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,<br>
+                           void *File) {<br>
+  uint32_t I;<br>
----------------<br>
I would rename the argument `void *WriterCtx` and then have a declaration `FILE *File = (FILE *)WriterCtx;` as the first thing in the function.<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D14859" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14859</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>