Re: [PATCH] gcov: make profile merging smarter

["Martin Liška" <mliska@suse.cz>]

On 10/5/21 12:04, Richard Biener wrote:
> On Mon, Oct 4, 2021 at 1:32 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 10/4/21 13:16, Richard Biener wrote:
>>> I meant in merge_one_data do not check ->stamp or ->checksum but instead rely
>>> on the counter merging code to detect mismatches (there's read_mismatch and
>>> read_error).  There's multiple things we can do when we run into those:
>>>
>>>    - when we did not actually merged any counter yet we could issue the
>>>      warning as before and drop the old data on the floor
>>>    - when we_did_  merge some counters already we could hard-error
>>>      (I suppose we can't roll-back merging that took place already)
>>>    - we could do the merging two-stage, first see whether the data matches
>>>      and only if it did perform the merging
>>
>> I've got your point, you are basically suggesting a fine grained merging
>> (function based). Huh, I don't like it much as it's typically a mistake
>> in the build setup that 2 objects (with a different checksum) want to emit
>> profile to the same .gcda file.
> 
> I agree, it's usually a mistake.
> 
>> My patch handles the obvious situation where an object file is built exactly
>> the same way (so no e.g. -O0 and -O2).
> 
> Yeah, but then the two profiles may not be related at all ...

Well, it's quite common case that one object file is then linked into multiple
binaries (e.g. util.o in a project). We collect also sum_max:
Sum of individual run max values.
which helps handling such a situation.

> 
>>>
>>> Note that all of the changes (including yours) have user-visible effects and
>>> the behavior is somewhat unobvious.  Not merging when the object was
>>> re-built is indeed the most obvious behavior so I'm not sure it's a good
>>> idea.  A new env variable to say whether to simply keep the_old_  data
>>> when merging in new data isn't possible would be another "fix" I guess?
>>
>> Even for a situation when checksum matches, but the timestamp is different?
>> Sure, we can provide env. variables that can tweak the behavior.
> 
> I suppose another distinguishing factor might be the name of the executable.

Well, at compile time, we don't know name of a final executable.

> 
> But yeah, in the end it's a fishy area ...
> 
> So I guess your originally posted patch might be the best way to go - can you
> try to amend the documentation as for the behavior with respect to
> re-compiling and profile merging?  I suppose that if you re-compile just
> a single .o you currently merge into all the other .o file counters but _not_
> into the newly compiled old counters.

Yes, I can update the documentation.

> That would make coverage off
> as well for incremental re-compiling?

Yes.

> 
> I only can find
> 
> @item
> Run the program on a representative workload to generate the arc profile
> information.  This may be repeated any number of times.  You can run
> concurrent instances of your program, and provided that the file system
> supports locking, the data files will be correctly updated.  Unless
> a strict ISO C dialect option is in effect, @code{fork} calls are
> detected and correctly handled without double counting.
> 
> but that's under -coverage, not sure if there's a better place to amend.
> 
> Note I see there's -fprofile-dir which eventually can be used to "fix"
> the SPEC issue as well?

We would have to provide a different option value of -fprofile-dir for both
binaries. That's something we can't easily do in a SPEC config file.

Let me update the documentation bits.

Martin

> 
> Richard.
> 
>> Cheers,
>> Martin
>>