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

[Xinliang David Li <davidxl at google dot com>]

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.

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

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