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

[Sean Silva via llvm-commits]

Thanks!

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.

-- Sean Silva

On Fri, Nov 20, 2015 at 8:22 PM, Xinliang David Li <davidxl at google.com>
wrote:

> 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
>
> On Fri, Nov 20, 2015 at 7:36 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> 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
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151120/22579d0e/attachment.html>