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

[Sean Silva via llvm-commits]

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

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

(if that is the issue then we could use a different callback to
llvmWriteProfDataImpl
to count the total size)

Actually, it seems a good idea to implement __llvm_profile_get_size_for_buffer
like that anyway.

-- Sean Silva


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