Re: [PATCH] Fix target clones (PR gcov-profile/85370).

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

On 07/26/2018 11:00 AM, Richard Biener wrote:
> On Thu, Jul 26, 2018 at 10:44 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 07/25/2018 03:50 PM, Richard Biener wrote:
>>> On Wed, Jul 25, 2018 at 3:38 PM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hi.
>>>>
>>>> Target clones have DECL_ARTIFICIAL set to 1, but we want to
>>>> provide --coverage for that. With patched GCC on can see:
>>>>
>>>>         -:    0:Source:pr85370.c
>>>>         -:    0:Graph:pr85370.gcno
>>>>         -:    0:Data:pr85370.gcda
>>>>         -:    0:Runs:1
>>>>         -:    0:Programs:1
>>>>         -:    1:__attribute__((target_clones("arch=slm","default")))
>>>>         1:    2:int foo1 (int a, int b) { // executed #### wrongly
>>>>         1:    3:  return a + b;
>>>>         -:    4:}
>>>> ------------------
>>>> foo1.arch_slm.0:
>>>>         0:    2:int foo1 (int a, int b) { // executed #### wrongly
>>>>         0:    3:  return a + b;
>>>>         -:    4:}
>>>> ------------------
>>>> foo1.default.1:
>>>>         1:    2:int foo1 (int a, int b) { // executed #### wrongly
>>>>         1:    3:  return a + b;
>>>>         -:    4:}
>>>> ------------------
>>>>         -:    5:
>>>>         1:    6:int foo2 (int a, int b) {
>>>>         1:    7:  return a + b;
>>>>         -:    8:}
>>>>         -:    9:
>>>>         1:   10:int main() {
>>>>         1:   11:  foo1(1, 1);
>>>>         1:   12:  foo2(1, 1);
>>>>         1:   13:  return 1;
>>>>         -:   14:}
>>>>
>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>> Will install in couple of days if no objection.
>>>
>>> I wonder if representing the clones as artificial but have their body be
>>> marked as inline instance of the original function works for gcov?
>>
>> Do you mean an inlined functions? If so, these are fine as gimple statements
>> that were inlined still point to original source code.
>>
>>  I think
>>> it should for debuggers.  A similar case is probably the
>>> static_constructors_and_destructors
>>
>> Actually static_c_a_d were motivation for exclusion. They have location set
>> to last line in source code and it's not intentional. Similarly implicit
>> ctors/dtors (e.g. Centering<3>::Centering(Centering<3> const&)) are ignored
>> as they don't have any real line of code in a source file.
>>
>>> function which has all ctors/dtors of static objects inlined into but itself is
>>> of course artificial.  Is that handled correctly?
>>
>> Hope I explained enough?
> 
> The question is what you like to see - looking at your figure above it
> looks like
> you want to see separate coverage for the different clones even when they
> are auto-generated by GCC?

Probably yes, I don't have any strong opinion here.

 Isn't that inconsistent with for example
> IPA-CP generated clones or inline instances?  Manual multiversions
> in source are already reported separately?

Yes, that would be reported separately.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2018-07-25  Martin Liska  <mliska@suse.cz>
>>>>
>>>>         PR gcov-profile/85370
>>>>         * coverage.c (coverage_begin_function): Do not mark target
>>>>         clones as artificial functions.
>>>> ---
>>>>  gcc/coverage.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>>
>>