On Fri, Nov 8, 2013 at 11:02 AM, Xinliang David Li <davi...@google.com> wrote: > On Fri, Nov 8, 2013 at 10:48 AM, Rong Xu <x...@google.com> wrote: >> On Fri, Nov 8, 2013 at 10:22 AM, Xinliang David Li <davi...@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) 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. 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 <x...@google.com> wrote: >>>> On Thu, Nov 7, 2013 at 9:40 AM, Joseph S. Myers <jos...@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 >>>>> jos...@codesourcery.com