Re: [RFC] libgcov.c re-factoring and offline profile-tool

[Rong Xu <xur at google dot com>]

On Fri, Nov 8, 2013 at 11:30 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Nov 8, 2013 at 11:21 AM, Rong Xu <xur@google.com> wrote:
>> On Fri, Nov 8, 2013 at 11:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Fri, Nov 8, 2013 at 10:48 AM, Rong Xu <xur@google.com> wrote:
>>>> On Fri, Nov 8, 2013 at 10:22 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> in libgcov-driver.c
>>>>>
>>>>>> /* Flag when the profile has already been dumped via __gcov_dump().  */
>>>>>> static int gcov_dump_complete;
>>>>>
>>>>>> inline void
>>>>>> set_gcov_dump_complete (void)
>>>>>>{
>>>>>   > gcov_dump_complete = 1;
>>>>>>}
>>>>>
>>>>>>inline void
>>>>>>reset_gcov_dump_complete (void)
>>>>>>{
>>>>>>  gcov_dump_complete = 0;
>>>>>>}
>>>>>
>>>>>
>>>>> These should be moved to libgcov-interface.c. Is there reason to not
>>>>> mark them as static?
>>>>
>>>> gcov_dump_complete is used in gcov_exit() which is in
>>>> libgcov-driver.c, but it's set in __gcov_dump and __gcov_reset, both
>>>> in libgcov-interface.c.
>>>> I want gcov_dump_complete a static. So I have to add to global
>>>> functions that accessible from libgcov-interface.c.
>>>> Another choice is to move __gcov_dump and __gcov_reset to
>>>> libgcov-driver.c and that will simplify the code.
>>>>
>>>
>>> Ok  then you should remove the 'inline' keyword for the set and reset
>>> functions and keep them in libgcov-driver.c.
>>
>> Will remove 'inline' keyword.
>>
>>>
>>>>>
>>>>> in libgcov-profiler.c, line 142
>>>>>
>>>>>> #if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
>>>>>> __thread
>>>>>
>>>>> trailing whilte space.
>>>>>
>>>>
>>>> Will fix.
>>>>
>>>>>
>>>>>>     || (VTABLE_USES_DESCRIPTORS && __gcov_indirect_call_callee
>>>>>>         && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
>>>>>
>>>>> trailing white space.
>>>>>
>>>>
>>>> Will fix.
>>>>
>>>>>
>>>>> in libgcov-merge.c
>>>>>
>>>>> 1) I don't think you need in_mem flag. For in memory merge, the source != NULL.
>>>>> 2) document the new source and weight parameter -- especially the weight.
>>>>
>>>> Will do.
>>>>
>>>>> 3) How do you deal with integer overflow ?
>>>>
>>>> This is good question. gcvo_type is (typically) long long. I thought
>>>> the count value will not be nearly close to the max of long long.
>>>> (We did see overflow in compiler, but that's because of the scaling --
>>>> some of the scaling factors are really large.)
>>>>
>>>
>>> Why not passing weight as a scaled PERCENT  (as branch prob) for the
>>> source? THis way, the merge tool does not need to do scaling twice.
>>>
>>
>> there are two weights, so unless you have two weights in the merge_add
>> functions, you have to
>> traverse the list twice. Do you mean pass addition parameters?
>>
>> Currently, merge tools is done this way:
>> merge (p1, p2, w1, w2)
>> first pass: merge_add (p1, p1, w1)
>> second pass: merge_add (p1, p2, w2)
>>
>
> Only need to pass the normalized the weight (in percent) for one
> profile source -- say norm_w2 : w2/(w1+w2), and the merge function can
> do this in one pass as norm_w1 = 1-norm_w2.
>
>
>> I initially had both weights in merge_add function. but later I found that
>> to deal with mis-match profile, I need to find out the gcov_info in p1
>> but not p2, (they need to multiply by w1 only).
>> So I choose the above structure.
>>
>> Another thing about the PERCENT is the inaccuracy for floating points.
>
> This is how profile update work in profile-use compilation -- so that
> should not be a big problem.
>
> Before you fix this, I'd like to hear what Honza and other reviewers think.
>
> thanks,
>
> David
>

OK. Let's get more comments on this before I re-work on this.
I also want to point out that the traversal the gcov-info list is
cheap. Even with the huge profile for google internal programs. The
majority of the time of the merge-tool is in read/write gcda files..

-Rong

>>
>> I have the scaling function in rewrite sub-command mainly for debug purpose:
>> I merge the same profile with a weight 1:9.
>> Then I scale the result profile by 0.1. I was expecting identical
>> profile as the source. But due to floating point inaccuracy, some
>> counters are off slightly.
>>
>>
>>> David
>>>
>>>
>>>
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Thu, Nov 7, 2013 at 3:34 PM, Rong Xu <xur@google.com> wrote:
>>>>>> On Thu, Nov 7, 2013 at 9:40 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>>>>>>> On Thu, 7 Nov 2013, Rong Xu wrote:
>>>>>>>
>>>>>>>> Thanks Joseph for these detailed comments/suggestions.
>>>>>>>> The fixed patch is attached to this email.
>>>>>>>> The only thing left out is the Texinfo manual. Do you mean this tool
>>>>>>>> should have its it's own texi file in gcc/doc?
>>>>>>>
>>>>>>> Its own texi file, probably included as a chapter by gcc.texi (like
>>>>>>> gcov.texi is).
>>>>>>
>>>>>> OK. will add this in.
>>>>>>
>>>>>> My last patch that changes the command handling is actually broken.
>>>>>> Please ignore that patch and use the one in this email.
>>>>>>
>>>>>> Also did some minor adjust of the code in libgcov.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Rong
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Joseph S. Myers
>>>>>>> joseph@codesourcery.com