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

[Xinliang David Li via llvm-commits]

On Fri, Nov 20, 2015 at 8:29 PM, Sean Silva <chisophugis at gmail.com> wrote:

> 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.
>

the problem is that buffer API currently does not support VP data, so the
problem would never be triggered. A test can always  be added  for future
protection of course.

David


> -- 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/603d49ef/attachment.html>