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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

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.

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(-)



[-- Attachment #2: 0001-Fix-target-clones-PR-gcov-profile-85370.patch --]
[-- Type: text/x-patch, Size: 660 bytes --]

diff --git a/gcc/coverage.c b/gcc/coverage.c
index da171c84d3c..bae6f5cafac 100644
--- a/gcc/coverage.c
+++ b/gcc/coverage.c
@@ -656,7 +656,8 @@ coverage_begin_function (unsigned lineno_checksum, unsigned cfg_checksum)
   gcov_write_unsigned (cfg_checksum);
   gcov_write_string (IDENTIFIER_POINTER
 		     (DECL_ASSEMBLER_NAME (current_function_decl)));
-  gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl));
+  gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl)
+		       && !DECL_FUNCTION_VERSIONED (current_function_decl));
   gcov_write_filename (xloc.file);
   gcov_write_unsigned (xloc.line);
   gcov_write_unsigned (xloc.column);

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

[Richard Biener]

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?  I think
it should for debuggers.  A similar case is probably the
static_constructors_and_destructors
function which has all ctors/dtors of static objects inlined into but itself is
of course artificial.  Is that handled correctly?

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(-)
>
>

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

[Martin Liška]

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?
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(-)
>>
>>

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

[Richard Biener]

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

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(-)
> >>
> >>
>

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

[Martin Liška]

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(-)
>>>>
>>>>
>>