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

[Sean Silva via llvm-commits]

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

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

What prevents using the buffer api with VP data? It should be using the
same codepath in llvmWriteProfDataImpl.

 Are we unable to calculate the size or something?

-- Sean Silva


>
> 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/27cd5f0c/attachment.html>