Remove code coverage counter for "empty" lines

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Remove code coverage counter for "empty" lines

via cfe-dev
Hi,

When making code coverage, there are some counters on closing braces.
In some case it's due to "return void" with a debugloc corresponding to '}' (see line 4) or due to cleanup stuff put at the end of a scope (see line 20, aaa's dtor is called).
Especially with c++, some "implicit" code (i.e. not written explicitly by the user) could be added at the beginning of a scope and at the end.
Here are two example of outputs produced by clang and gcc.

With clang 7:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
-:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
-:    9:void foo(int K) {
1:   10:}
-:   11:
-:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
1:   19:}

With gcc 8:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
1:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

So I'd like to have coverage for lines which contain only "explicit" code to have a clear information to give to different kinds of people (I spent myself some time to understand why I had coverage on line like "} // namespace foo").
So we could add an option in clang (no idea of the name right now) to allow user to have coverage for explicit code.
And so have this output:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
-:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
-:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

Calixte

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Remove code coverage counter for "empty" lines

via cfe-dev
(+ a few folks who've been involved in the reviews, etc)

On Mon, Sep 24, 2018 at 12:34 PM Calixte Denizet via cfe-dev <[hidden email]> wrote:
Hi,

When making code coverage, there are some counters on closing braces.
In some case it's due to "return void" with a debugloc corresponding to '}' (see line 4) or due to cleanup stuff put at the end of a scope (see line 20, aaa's dtor is called).
Especially with c++, some "implicit" code (i.e. not written explicitly by the user) could be added at the beginning of a scope and at the end.
Here are two example of outputs produced by clang and gcc.

With clang 7:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
-:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
-:    9:void foo(int K) {
1:   10:}
-:   11:
-:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
1:   19:}

With gcc 8:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
1:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

So I'd like to have coverage for lines which contain only "explicit" code to have a clear information to give to different kinds of people (I spent myself some time to understand why I had coverage on line like "} // namespace foo").
So we could add an option in clang (no idea of the name right now) to allow user to have coverage for explicit code.
And so have this output:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
-:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
-:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

Calixte
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Remove code coverage counter for "empty" lines

via cfe-dev
Hi,

On Sep 25, 2018, at 12:46 PM, David Blaikie <[hidden email]> wrote:

(+ a few folks who've been involved in the reviews, etc)

On Mon, Sep 24, 2018 at 12:34 PM Calixte Denizet via cfe-dev <[hidden email]> wrote:
Hi,

When making code coverage, there are some counters on closing braces.
In some case it's due to "return void" with a debugloc corresponding to '}' (see line 4) or due to cleanup stuff put at the end of a scope (see line 20, aaa's dtor is called).
Especially with c++, some "implicit" code (i.e. not written explicitly by the user) could be added at the beginning of a scope and at the end.
Here are two example of outputs produced by clang and gcc.

With clang 7:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
-:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
-:    9:void foo(int K) {
1:   10:}
-:   11:
-:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
1:   19:}

With gcc 8:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
1:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

So I'd like to have coverage for lines which contain only "explicit" code to have a clear information to give to different kinds of people (I spent myself some time to understand why I had coverage on line like "} // namespace foo”).

Makes sense, although I’ll note that there are cases where hiding coverage stats on closing braces might be doing a disservice, like:

void foo() {
  if (…)
    return;
  if (…)
    return;
} //< The coverage here might be interesting if you care about how often you don’t return early.

You can figure it out by subtracting the coverage counts on return statements from the function entry count, but that’s work the coverage tool could do for you.

So we could add an option in clang (no idea of the name right now) to allow user to have coverage for explicit code.

I’d rather not have a flag to control this behavior. I imagine that most users of a coverage tool will never change its default behavior. From a maintenance perspective, it’s easier to test one coverage mode than two.

You’re making a compelling argument that hiding coverage stats on implicit code is the right thing to do (if for no other reason than “it’s what gcc does”). If most others agree with you, I’d rather have such a change made unconditionally even though it’s not my personal preference.

Just my 2 cents — happy to defer to others maintaining/using gcov.

thanks,
vedant


And so have this output:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
-:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
-:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

Calixte
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Remove code coverage counter for "empty" lines

via cfe-dev
I have no understanding of any of the file formats involved, but it seems like it would be the kind of thing we'd record in the information about the source location, and then the tool that generates text or HTML can display it how it likes at the end. That's why I support putting the info in the LLVM IR format, since we'll need to track the bit through instrumentation.

If the file format is too hard to extend, then I wouldn't mind adding an extra option for people who want to tweak our instrumentation.

On Wed, Sep 26, 2018 at 3:10 PM Vedant Kumar <[hidden email]> wrote:
Hi,

On Sep 25, 2018, at 12:46 PM, David Blaikie <[hidden email]> wrote:

(+ a few folks who've been involved in the reviews, etc)

On Mon, Sep 24, 2018 at 12:34 PM Calixte Denizet via cfe-dev <[hidden email]> wrote:
Hi,

When making code coverage, there are some counters on closing braces.
In some case it's due to "return void" with a debugloc corresponding to '}' (see line 4) or due to cleanup stuff put at the end of a scope (see line 20, aaa's dtor is called).
Especially with c++, some "implicit" code (i.e. not written explicitly by the user) could be added at the beginning of a scope and at the end.
Here are two example of outputs produced by clang and gcc.

With clang 7:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
-:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
-:    9:void foo(int K) {
1:   10:}
-:   11:
-:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
1:   19:}

With gcc 8:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
1:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

So I'd like to have coverage for lines which contain only "explicit" code to have a clear information to give to different kinds of people (I spent myself some time to understand why I had coverage on line like "} // namespace foo”).

Makes sense, although I’ll note that there are cases where hiding coverage stats on closing braces might be doing a disservice, like:

void foo() {
  if (…)
    return;
  if (…)
    return;
} //< The coverage here might be interesting if you care about how often you don’t return early.

You can figure it out by subtracting the coverage counts on return statements from the function entry count, but that’s work the coverage tool could do for you.

So we could add an option in clang (no idea of the name right now) to allow user to have coverage for explicit code.

I’d rather not have a flag to control this behavior. I imagine that most users of a coverage tool will never change its default behavior. From a maintenance perspective, it’s easier to test one coverage mode than two.

You’re making a compelling argument that hiding coverage stats on implicit code is the right thing to do (if for no other reason than “it’s what gcc does”). If most others agree with you, I’d rather have such a change made unconditionally even though it’s not my personal preference.

Just my 2 cents — happy to defer to others maintaining/using gcov.

thanks,
vedant


And so have this output:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
-:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
-:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

Calixte
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Remove code coverage counter for "empty" lines

via cfe-dev
@reid, Currently, the GCNO/GCDA format are the same as in gcc 4.2 (https://llvm.org/docs/CommandGuide/llvm-cov.html)
So adding extra data will break the compatibility with gcc 4.2.

@vedant, an example of what we've currently with a code similar to yours:
-:    0:Source:d.c
-:    0:Graph:d.gcno
-:    0:Data:d.gcda
-:    0:Runs:1
-:    0:Programs:1
-:    1:int foo(int x) {
3:    2:    if (x == 0) {
1:    3:       return 1;
2:    4:    } else if (x == 1) {
1:    5:       return 2;
-:    6:    }
-:    7:
1:    8:   return 3;
3:    9:}
-:   10:
-:   11:int main(int argc, char ** argv) {
1:   12:    foo(0);
1:   13:    foo(1);
1:   14:    foo(2);
-:   15:
1:   16:    return 3;
-:   17:}

I'm not sure that the 3 on line 9 is so useful because finally it relies on how it's implemented behind (in the IR we only have one final ret instruction).
From my point of view, a code coverage report should be easily understandable (I mean no need to do maths) and not rely on compiler details.
I agree with you on the fact that having two modes is painful and from my point of view when I made this patch (reverted) in clang, I had the impress to fix a bug and not to add a feature.
So what could we do to go ahead ?


Le lun. 1 oct. 2018 à 23:14, Reid Kleckner <[hidden email]> a écrit :
I have no understanding of any of the file formats involved, but it seems like it would be the kind of thing we'd record in the information about the source location, and then the tool that generates text or HTML can display it how it likes at the end. That's why I support putting the info in the LLVM IR format, since we'll need to track the bit through instrumentation.

If the file format is too hard to extend, then I wouldn't mind adding an extra option for people who want to tweak our instrumentation.

On Wed, Sep 26, 2018 at 3:10 PM Vedant Kumar <[hidden email]> wrote:
Hi,

On Sep 25, 2018, at 12:46 PM, David Blaikie <[hidden email]> wrote:

(+ a few folks who've been involved in the reviews, etc)

On Mon, Sep 24, 2018 at 12:34 PM Calixte Denizet via cfe-dev <[hidden email]> wrote:
Hi,

When making code coverage, there are some counters on closing braces.
In some case it's due to "return void" with a debugloc corresponding to '}' (see line 4) or due to cleanup stuff put at the end of a scope (see line 20, aaa's dtor is called).
Especially with c++, some "implicit" code (i.e. not written explicitly by the user) could be added at the beginning of a scope and at the end.
Here are two example of outputs produced by clang and gcc.

With clang 7:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
-:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
-:    9:void foo(int K) {
1:   10:}
-:   11:
-:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
1:   19:}

With gcc 8:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
1:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
1:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

So I'd like to have coverage for lines which contain only "explicit" code to have a clear information to give to different kinds of people (I spent myself some time to understand why I had coverage on line like "} // namespace foo”).

Makes sense, although I’ll note that there are cases where hiding coverage stats on closing braces might be doing a disservice, like:

void foo() {
  if (…)
    return;
  if (…)
    return;
} //< The coverage here might be interesting if you care about how often you don’t return early.

You can figure it out by subtracting the coverage counts on return statements from the function entry count, but that’s work the coverage tool could do for you.

So we could add an option in clang (no idea of the name right now) to allow user to have coverage for explicit code.

I’d rather not have a flag to control this behavior. I imagine that most users of a coverage tool will never change its default behavior. From a maintenance perspective, it’s easier to test one coverage mode than two.

You’re making a compelling argument that hiding coverage stats on implicit code is the right thing to do (if for no other reason than “it’s what gcc does”). If most others agree with you, I’d rather have such a change made unconditionally even though it’s not my personal preference.

Just my 2 cents — happy to defer to others maintaining/using gcov.

thanks,
vedant


And so have this output:
-:    1:struct A {
-:    2:
1:    3:    A() {}
-:    4:   
1:    5:    ~A() {
-:    6:    }
-:    7:};
-:    8:
1:    9:void foo(int K) {
-:   10:}
-:   11:
1:   12:int main() {
1:   13:    A aaa;
1:   14:    int x = 1;
1:   15:    foo(x);
1:   16:    x = x + 2;
-:   17:
1:   18:    return x;
-:   19:}

Calixte
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev