On Fri, Nov 8, 2013 at 11:30 AM, Xinliang David Li <davi...@google.com> wrote: > On Fri, Nov 8, 2013 at 11:21 AM, Rong Xu <x...@google.com> wrote: >> 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) >> > > 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 <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