New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: disable -O3 for C++ coverage #12406

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@addaleax
Member

addaleax commented Apr 13, 2017

The cflags for --coverage included -O0 so far, but that was
overridden by a later -O3. Resolve that by adding
'cflags!': [ '-O3' ] and increase coverage accuracy.

CI: https://ci.nodejs.org/job/node-test-commit/9087/

/cc @mhdawson @CurryKitten

build: disable -O3 for C++ coverage
The `cflags` for `--coverage` included `-O0` so far, but that was
overridden by a later `-O3`. Resolve that by adding
`'cflags!': [ '-O3' ]` and increase coverage accuracy.
@refack

refack approved these changes Apr 13, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

P.S. did you notice that when coverage == 'false' -O3 is added twice

...
g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3
...
Member

refack commented Apr 13, 2017

P.S. did you notice that when coverage == 'false' -O3 is added twice

...
g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3
...
@refack

This comment has been minimized.

Show comment
Hide comment
@refack
Member

refack commented Apr 13, 2017

P.S. wouldn't this be more interesting: https://ci.nodejs.org/job/node-test-commit-linux-coverage/221/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 13, 2017

Member

P.S. did you notice that when coverage == 'false' -O3 is added twice

Yup… I guess gyp ist just being gyp.

P.S. wouldn't this be more interesting: https://ci.nodejs.org/job/node-test-commit-linux-coverage/221/

Yes! I didn’t know that existed. :)

Member

addaleax commented Apr 13, 2017

P.S. did you notice that when coverage == 'false' -O3 is added twice

Yup… I guess gyp ist just being gyp.

P.S. wouldn't this be more interesting: https://ci.nodejs.org/job/node-test-commit-linux-coverage/221/

Yes! I didn’t know that existed. :)

@mhdawson

That would explain some of the strange results I was seeing when looking at the N-API coverage. Thanks. LGTM.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

In term of running the job for coverage we've not generally publicized that you can start a job on your own branch as it will push the results out to the website and that might lead to some confusing up/downs on coverage.nodejs.org. Having said that, this is probably a case where it does make sense.

Member

mhdawson commented Apr 13, 2017

In term of running the job for coverage we've not generally publicized that you can start a job on your own branch as it will push the results out to the website and that might lead to some confusing up/downs on coverage.nodejs.org. Having said that, this is probably a case where it does make sense.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

From Job 221

Javascript coverage %: 89.9%
C++ coverage %: 86.6%

Makes sense?

Member

refack commented Apr 13, 2017

From Job 221

Javascript coverage %: 89.9%
C++ coverage %: 86.6%

Makes sense?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 13, 2017

Member

@mhdawson Oh, yes, thanks for explaining that it does that! 😄 Did this end up getting pushed to the website, though? I don’t see it at https://coverage.nodejs.org/, but the run seems to have completed. (If it does end up there: Maybe we should merge this sooner than later? Otherwise it just gets more confusing, and this patch is not, like, overly complex or anything…)

Member

addaleax commented Apr 13, 2017

@mhdawson Oh, yes, thanks for explaining that it does that! 😄 Did this end up getting pushed to the website, though? I don’t see it at https://coverage.nodejs.org/, but the run seems to have completed. (If it does end up there: Maybe we should merge this sooner than later? Otherwise it just gets more confusing, and this patch is not, like, overly complex or anything…)

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

as it will push the results out to the website

Oops 😟 I'm used to CIs being very selective in terms of publishing from non-master

Member

refack commented Apr 13, 2017

as it will push the results out to the website

Oops 😟 I'm used to CIs being very selective in terms of publishing from non-master

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 13, 2017

Member

Makes sense?

Yes. The overall C++ coverage is a bit lower than previously, but that seems to be because the compiler now adds coverage support for more lines that were previously optimized away (for example, line 23 in base64.h which is obviously pointless to actually execute).

Member

addaleax commented Apr 13, 2017

Makes sense?

Yes. The overall C++ coverage is a bit lower than previously, but that seems to be because the compiler now adds coverage support for more lines that were previously optimized away (for example, line 23 in base64.h which is obviously pointless to actually execute).

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

On the site first line is:

Date (UTC) HEAD JS Coverage C++ Coverage
13/04/2017 03:18 4ac3ef5 89.90 % 88.50 %
Member

refack commented Apr 13, 2017

On the site first line is:

Date (UTC) HEAD JS Coverage C++ Coverage
13/04/2017 03:18 4ac3ef5 89.90 % 88.50 %
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

@refack that is a a good idea, I should look at updating the job to only publish if it was started off master, will add that to my TODO list.

Member

mhdawson commented Apr 13, 2017

@refack that is a a good idea, I should look at updating the job to only publish if it was started off master, will add that to my TODO list.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

@refack that is a a good idea, I should look at updating the job to only publish if it was started off master, will add that to my TODO list.

If you point me to where, I'd be happy to help.

Member

refack commented Apr 13, 2017

@refack that is a a good idea, I should look at updating the job to only publish if it was started off master, will add that to my TODO list.

If you point me to where, I'd be happy to help.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

In terms of the difference in coverage numbers it just sounds like something we'll have to live with, we can probably let people know why this change occurred and move on unless we have a better idea.

Member

mhdawson commented Apr 13, 2017

In terms of the difference in coverage numbers it just sounds like something we'll have to live with, we can probably let people know why this change occurred and move on unless we have a better idea.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 13, 2017

Member

@mhdawson See #12406 (comment) – I just looked at a few examples but it would make sense if that was the general reason for it

Member

addaleax commented Apr 13, 2017

@mhdawson See #12406 (comment) – I just looked at a few examples but it would make sense if that was the general reason for it

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

@refack the ability to change the CI jobs is limited to the build team except for some WG specific jobs so you won't be able to edit it. But if you have some sample jenkins shell script code (we currently only generate the results on linux) that does the check from another project send it to me and I can probably just paste it in. Otherwise I don't think it will take me too long to put it together.

Member

mhdawson commented Apr 13, 2017

@refack the ability to change the CI jobs is limited to the build team except for some WG specific jobs so you won't be able to edit it. But if you have some sample jenkins shell script code (we currently only generate the results on linux) that does the check from another project send it to me and I can probably just paste it in. Otherwise I don't think it will take me too long to put it together.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

@addaleax I was responding you your comment (guess I could have said that :)), makes sense to me.

Member

mhdawson commented Apr 13, 2017

@addaleax I was responding you your comment (guess I could have said that :)), makes sense to me.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Apr 13, 2017

Member

The results do get pushed at an interval, I can't remember exactly now but its at least once a night (might be every 6 hours, I could look it up).

I could go delete the result, but I'd vote for just pushing the change.

Member

mhdawson commented Apr 13, 2017

The results do get pushed at an interval, I can't remember exactly now but its at least once a night (might be every 6 hours, I could look it up).

I could go delete the result, but I'd vote for just pushing the change.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

I'm landing some, I'll land this as well.

Member

refack commented Apr 13, 2017

I'm landing some, I'll land this as well.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 13, 2017

Member

Landed in ea44b8b

Member

refack commented Apr 13, 2017

Landed in ea44b8b

@refack refack closed this Apr 13, 2017

refack added a commit that referenced this pull request Apr 13, 2017

build: disable -O3 for C++ coverage
The `cflags` for `--coverage` included `-O0` so far, but that was
overridden by a later `-O3`. Resolve that by adding
`'cflags!': [ '-O3' ]` and increase coverage accuracy.

Ref: https://coverage.nodejs.org/
PR-URL: #12406
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax deleted the addaleax:cov-no-o3 branch Apr 13, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

gibfahn added a commit that referenced this pull request Jun 18, 2017

build: disable -O3 for C++ coverage
The `cflags` for `--coverage` included `-O0` so far, but that was
overridden by a later `-O3`. Resolve that by adding
`'cflags!': [ '-O3' ]` and increase coverage accuracy.

Ref: https://coverage.nodejs.org/
PR-URL: #12406
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Jun 20, 2017

build: disable -O3 for C++ coverage
The `cflags` for `--coverage` included `-O0` so far, but that was
overridden by a later `-O3`. Resolve that by adding
`'cflags!': [ '-O3' ]` and increase coverage accuracy.

Ref: https://coverage.nodejs.org/
PR-URL: #12406
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Jul 11, 2017

build: disable -O3 for C++ coverage
The `cflags` for `--coverage` included `-O0` so far, but that was
overridden by a later `-O3`. Resolve that by adding
`'cflags!': [ '-O3' ]` and increase coverage accuracy.

Ref: https://coverage.nodejs.org/
PR-URL: #12406
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

Merged

v6.11.2 proposal #14356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment